[PATCH v2] git-merge: ignore space support

[PATCH v2] git-merge: ignore space support

[Justin Frankel]

git-merge and git-rebase can now be passed -Xignore-space-at-eol,
-Xignore-space-change, -Xignore-all-space, -Xpatience options.

This is for the next/ branch (as the ll-merge flag interface has been
cleaned up). This version of the patch is signed-off and passes git diff 
--check. 

Signed-off-by: Justin Frankel <justin@cockos.com>
---
 builtin/merge-recursive.c |    9 +++++++++
 builtin/merge.c           |    9 +++++++++
 ll-merge.c                |    1 +
 ll-merge.h                |   15 ++++++++++++++-
 merge-recursive.c         |    2 +-
 merge-recursive.h         |    1 +
 6 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index c2d4677..7cbc913 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -2,6 +2,7 @@
 #include "commit.h"
 #include "tag.h"
 #include "merge-recursive.h"
+#include "xdiff-interface.h"
 
 static const char *better_branch_name(const char *branch)
 {
@@ -49,6 +50,14 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 				o.renormalize = 1;
 			else if (!strcmp(arg+2, "no-renormalize"))
 				o.renormalize = 0;
+			else if (!strcmp(arg+2, "ignore-all-space"))
+				o.xdl_opts |= XDF_IGNORE_WHITESPACE;
+			else if (!strcmp(arg+2, "ignore-space-change"))
+				o.xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
+			else if (!strcmp(arg+2, "ignore-space-at-eol"))
+				o.xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
+			else if (!strcmp(arg+2, "patience"))
+				o.xdl_opts |= XDF_PATIENCE_DIFF;
 			else
 				die("Unknown option %s", arg);
 			continue;
diff --git a/builtin/merge.c b/builtin/merge.c
index 4e4ec89..3fe5dc7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -25,6 +25,7 @@
 #include "help.h"
 #include "merge-recursive.h"
 #include "resolve-undo.h"
+#include "xdiff-interface.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -647,6 +648,14 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 				o.renormalize = 1;
 			else if (!strcmp(xopts[x], "no-renormalize"))
 				o.renormalize = 0;
+			else if (!strcmp(xopts[x], "ignore-all-space"))
+				o.xdl_opts |= XDF_IGNORE_WHITESPACE;
+			else if (!strcmp(xopts[x], "ignore-space-change"))
+				o.xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
+			else if (!strcmp(xopts[x], "ignore-space-at-eol"))
+				o.xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
+			else if (!strcmp(xopts[x], "patience"))
+				o.xdl_opts |= XDF_PATIENCE_DIFF;
 			else
 				die("Unknown option for merge-recursive: -X%s", xopts[x]);
 		}
diff --git a/ll-merge.c b/ll-merge.c
index 6bb3095..8a0a076 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -80,6 +80,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	memset(&xmp, 0, sizeof(xmp));
 	xmp.level = XDL_MERGE_ZEALOUS;
 	xmp.favor = ll_opt_favor(flag);
+	xmp.xpp.flags = ll_opt_xdl_opt(flag);
 	if (git_xmerge_style >= 0)
 		xmp.style = git_xmerge_style;
 	if (marker_size > 0)
diff --git a/ll-merge.h b/ll-merge.h
index ff7ca87..cf2d7b9 100644
--- a/ll-merge.h
+++ b/ll-merge.h
@@ -7,19 +7,32 @@
 
 #define LL_OPT_VIRTUAL_ANCESTOR	(1 << 0)
 #define LL_OPT_FAVOR_MASK	((1 << 1) | (1 << 2))
-#define LL_OPT_FAVOR_SHIFT 1
+#define LL_OPT_FAVOR_SHIFT 	1
 #define LL_OPT_RENORMALIZE	(1 << 3)
+#define LL_OPT_XDL_MASK 	(0x3F << 4)
+#define LL_OPT_XDL_SHIFT 	4
 
 static inline int ll_opt_favor(int flag)
 {
 	return (flag & LL_OPT_FAVOR_MASK) >> LL_OPT_FAVOR_SHIFT;
 }
 
+static inline int ll_opt_xdl_opt(int flag)
+{
+	return ((flag & LL_OPT_XDL_MASK) >> LL_OPT_XDL_SHIFT);
+}
+
 static inline int create_ll_flag(int favor)
 {
 	return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK);
 }
 
+static inline int create_ll_flag_xdl_opt(int favor, int xdl_opt)
+{
+	return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK) |
+		((xdl_opt << LL_OPT_XDL_SHIFT) & LL_OPT_XDL_MASK);
+}
+
 int ll_merge(mmbuffer_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
diff --git a/merge-recursive.c b/merge-recursive.c
index aadd48c..506c9db 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -646,7 +646,7 @@ static int merge_3way(struct merge_options *o,
 				&src1, name1, &src2, name2,
 				((o->call_depth ? LL_OPT_VIRTUAL_ANCESTOR : 0) |
 				 (o->renormalize ? LL_OPT_RENORMALIZE : 0) |
-				 create_ll_flag(favor)));
+				 create_ll_flag_xdl_opt(favor,o->xdl_opts)));
 
 	free(name1);
 	free(name2);
diff --git a/merge-recursive.h b/merge-recursive.h
index 196f053..47f718f 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -13,6 +13,7 @@ struct merge_options {
 		MERGE_RECURSIVE_THEIRS
 	} recursive_variant;
 	const char *subtree_shift;
+	int xdl_opts;
 	unsigned buffer_output : 1;
 	unsigned renormalize : 1;
 	int verbosity;
-- 
1.7.0.2.msysgit.0

Re: [PATCH v2] git-merge: ignore space support

[Jonathan Nieder]

Justin Frankel wrote:

> This is for the next/ branch

Applies on top of jn/merge-renormalize.  Thanks for the reroll.

> --- a/builtin/merge-recursive.c
> +++ b/builtin/merge-recursive.c
> @@ -49,6 +50,14 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
>  				o.renormalize = 1;
>  			else if (!strcmp(arg+2, "no-renormalize"))
>  				o.renormalize = 0;
> +			else if (!strcmp(arg+2, "ignore-all-space"))
> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE;
> +			else if (!strcmp(arg+2, "ignore-space-change"))
> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
> +			else if (!strcmp(arg+2, "ignore-space-at-eol"))
> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
> +			else if (!strcmp(arg+2, "patience"))
> +				o.xdl_opts |= XDF_PATIENCE_DIFF;

So this teaches merge-recursive and ll-merge to accept the xdiff
whitespace flags (plus "patience").

> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -647,6 +648,14 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
>  				o.renormalize = 1;
>  			else if (!strcmp(xopts[x], "no-renormalize"))
>  				o.renormalize = 0;
> +			else if (!strcmp(xopts[x], "ignore-all-space"))
> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE;
> +			else if (!strcmp(xopts[x], "ignore-space-change"))
> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
> +			else if (!strcmp(xopts[x], "ignore-space-at-eol"))
> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
> +			else if (!strcmp(xopts[x], "patience"))
> +				o.xdl_opts |= XDF_PATIENCE_DIFF;

It's tempting to fix this code duplication once and for all.

> +++ b/ll-merge.h
> @@ -7,19 +7,32 @@
>  
>  #define LL_OPT_VIRTUAL_ANCESTOR	(1 << 0)
>  #define LL_OPT_FAVOR_MASK	((1 << 1) | (1 << 2))
> -#define LL_OPT_FAVOR_SHIFT 1
> +#define LL_OPT_FAVOR_SHIFT 	1
>  #define LL_OPT_RENORMALIZE	(1 << 3)
> +#define LL_OPT_XDL_MASK 	(0x3F << 4)
> +#define LL_OPT_XDL_SHIFT 	4

The space-instead-of-tab was intentional: the idea is that this is a
table:

	name			bits
	VIRTUAL_ANCESTOR	0
	FAVOR_MASK		1, 2
	FAVOR_SHIFT = 1
	RENORMALIZE		3
	WHITESPACE_MASK		4, 5, 6
	WHITESPACE_SHIFT = 4
	PATIENCE		7

and since a shift is not a bitmask it does not deserve an entry in the
table.

The flag layout worries me a bit because presumably the next extension
would get bit 8, and so on.  What happens if xdiff learns another
whitespace-ignoring mode?

>  
>  static inline int ll_opt_favor(int flag)
>  {
>  	return (flag & LL_OPT_FAVOR_MASK) >> LL_OPT_FAVOR_SHIFT;
>  }
>  
> +static inline int ll_opt_xdl_opt(int flag)
> +{
> +	return ((flag & LL_OPT_XDL_MASK) >> LL_OPT_XDL_SHIFT);
> +}

Style nit: parentheses around a return value are not needed.

>  static inline int create_ll_flag(int favor)
>  {
>  	return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK);
>  }

Looks like I am guilty of the same thing.
 
> +static inline int create_ll_flag_xdl_opt(int favor, int xdl_opt)
> +{
> +	return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK) |
> +		((xdl_opt << LL_OPT_XDL_SHIFT) & LL_OPT_XDL_MASK);
> +}

This function (like its cousin create_ll_flag) is ugly, as Bert
noticed before.  I am starting to wonder if it would not be simpler
to use

 extern int ll_merge(..., const struct ll_merge_options *opts);

treating NULL as a request for the default options.

In other words, how about something like this?

-- 8< --
Subject: ll-merge: replace flag argument with options struct

Keeping track of the flag bits is proving more trouble than it's
worth.  Instead, use a pointer to an options struct like most similar
APIs do.

Callers with no special requests can pass NULL to request the default
options.

Cc: Bert Wesarg <bert.wesarg@googlemail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/checkout.c |    2 +-
 ll-merge.c         |   42 ++++++++++++++++++++++++++----------------
 ll-merge.h         |   21 ++++++---------------
 merge-file.c       |    2 +-
 merge-recursive.c  |   22 +++++++++++-----------
 rerere.c           |    2 +-
 6 files changed, 46 insertions(+), 45 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 24b67d5..4d36b28 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -155,7 +155,7 @@ static int checkout_merged(int pos, struct checkout *state)
 	 * merge.renormalize set, too
 	 */
 	status = ll_merge(&result_buf, path, &ancestor, "base",
-			  &ours, "ours", &theirs, "theirs", 0);
+			  &ours, "ours", &theirs, "theirs", NULL);
 	free(ancestor.ptr);
 	free(ours.ptr);
 	free(theirs.ptr);
diff --git a/ll-merge.c b/ll-merge.c
index 6bb3095..98f353a 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -18,7 +18,7 @@ typedef int (*ll_merge_fn)(const struct ll_merge_driver *,
 			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
-			   int flag,
+			   const struct ll_merge_options *opts,
 			   int marker_size);
 
 struct ll_merge_driver {
@@ -39,14 +39,15 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
-			   int flag, int marker_size)
+			   const struct ll_merge_options *opts,
+			   int marker_size)
 {
 	/*
 	 * The tentative merge result is "ours" for the final round,
 	 * or common ancestor for an internal merge.  Still return
 	 * "conflicted merge" status.
 	 */
-	mmfile_t *stolen = (flag & LL_OPT_VIRTUAL_ANCESTOR) ? orig : src1;
+	mmfile_t *stolen = opts->virtual_ancestor ? orig : src1;
 
 	result->ptr = stolen->ptr;
 	result->size = stolen->size;
@@ -60,7 +61,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
-			int flag, int marker_size)
+			const struct ll_merge_options *opts,
+			int marker_size)
 {
 	xmparam_t xmp;
 
@@ -74,12 +76,12 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 				       orig, orig_name,
 				       src1, name1,
 				       src2, name2,
-				       flag, marker_size);
+				       opts, marker_size);
 	}
 
 	memset(&xmp, 0, sizeof(xmp));
 	xmp.level = XDL_MERGE_ZEALOUS;
-	xmp.favor = ll_opt_favor(flag);
+	xmp.favor = opts->variant;
 	if (git_xmerge_style >= 0)
 		xmp.style = git_xmerge_style;
 	if (marker_size > 0)
@@ -96,14 +98,15 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,
 			  mmfile_t *orig, const char *orig_name,
 			  mmfile_t *src1, const char *name1,
 			  mmfile_t *src2, const char *name2,
-			  int flag, int marker_size)
+			  const struct ll_merge_options *opts,
+			  int marker_size)
 {
 	/* Use union favor */
-	flag &= ~LL_OPT_FAVOR_MASK;
-	flag |= create_ll_flag(XDL_MERGE_FAVOR_UNION);
+	struct ll_merge_options o = *opts;
+	o.variant = XDL_MERGE_FAVOR_UNION;
 	return ll_xdl_merge(drv_unused, result, path_unused,
 			    orig, NULL, src1, NULL, src2, NULL,
-			    flag, marker_size);
+			    &o, marker_size);
 	return 0;
 }
 
@@ -136,7 +139,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
-			int flag, int marker_size)
+			const struct ll_merge_options *opts,
+			int marker_size)
 {
 	char temp[4][50];
 	struct strbuf cmd = STRBUF_INIT;
@@ -337,15 +341,21 @@ int ll_merge(mmbuffer_t *result_buf,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
 	     mmfile_t *theirs, const char *their_label,
-	     int flag)
+	     const struct ll_merge_options *opts)
 {
 	static struct git_attr_check check[2];
 	const char *ll_driver_name = NULL;
 	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	const struct ll_merge_driver *driver;
-	int virtual_ancestor = flag & LL_OPT_VIRTUAL_ANCESTOR;
 
-	if (flag & LL_OPT_RENORMALIZE) {
+	if (!opts) {
+		struct ll_merge_options default_opts = {0};
+		return ll_merge(result_buf, path, ancestor, ancestor_label,
+				ours, our_label, theirs, their_label,
+				&default_opts);
+	}
+
+	if (opts->renormalize) {
 		normalize_file(ancestor, path);
 		normalize_file(ours, path);
 		normalize_file(theirs, path);
@@ -359,11 +369,11 @@ int ll_merge(mmbuffer_t *result_buf,
 		}
 	}
 	driver = find_ll_merge_driver(ll_driver_name);
-	if (virtual_ancestor && driver->recursive)
+	if (opts->virtual_ancestor && driver->recursive)
 		driver = find_ll_merge_driver(driver->recursive);
 	return driver->fn(driver, result_buf, path, ancestor, ancestor_label,
 			  ours, our_label, theirs, their_label,
-			  flag, marker_size);
+			  opts, marker_size);
 }
 
 int ll_merge_marker_size(const char *path)
diff --git a/ll-merge.h b/ll-merge.h
index ff7ca87..4b707f0 100644
--- a/ll-merge.h
+++ b/ll-merge.h
@@ -5,27 +5,18 @@
 #ifndef LL_MERGE_H
 #define LL_MERGE_H
 
-#define LL_OPT_VIRTUAL_ANCESTOR	(1 << 0)
-#define LL_OPT_FAVOR_MASK	((1 << 1) | (1 << 2))
-#define LL_OPT_FAVOR_SHIFT 1
-#define LL_OPT_RENORMALIZE	(1 << 3)
-
-static inline int ll_opt_favor(int flag)
-{
-	return (flag & LL_OPT_FAVOR_MASK) >> LL_OPT_FAVOR_SHIFT;
-}
-
-static inline int create_ll_flag(int favor)
-{
-	return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK);
-}
+struct ll_merge_options {
+	unsigned virtual_ancestor : 1;
+	unsigned variant : 2;	/* favor ours, favor theirs, or union merge */
+	unsigned renormalize : 1;
+};
 
 int ll_merge(mmbuffer_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
 	     mmfile_t *theirs, const char *their_label,
-	     int flag);
+	     const struct ll_merge_options *opts);
 
 int ll_merge_marker_size(const char *path);
 
diff --git a/merge-file.c b/merge-file.c
index db4d0d5..f7f4533 100644
--- a/merge-file.c
+++ b/merge-file.c
@@ -37,7 +37,7 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our
 	 * common ancestor.
 	 */
 	merge_status = ll_merge(&res, path, base, NULL,
-				our, ".our", their, ".their", 0);
+				our, ".our", their, ".their", NULL);
 	if (merge_status < 0)
 		return NULL;
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 762b549..ed09785 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -608,22 +608,25 @@ static int merge_3way(struct merge_options *o,
 		      const char *branch2)
 {
 	mmfile_t orig, src1, src2;
+	struct ll_merge_options ll_opts = {0};
 	char *base_name, *name1, *name2;
 	int merge_status;
-	int favor;
 
-	if (o->call_depth)
-		favor = 0;
-	else {
+	ll_opts.renormalize = o->renormalize;
+
+	if (o->call_depth) {
+		ll_opts.virtual_ancestor = 1;
+		ll_opts.variant = 0;
+	} else {
 		switch (o->recursive_variant) {
 		case MERGE_RECURSIVE_OURS:
-			favor = XDL_MERGE_FAVOR_OURS;
+			ll_opts.variant = XDL_MERGE_FAVOR_OURS;
 			break;
 		case MERGE_RECURSIVE_THEIRS:
-			favor = XDL_MERGE_FAVOR_THEIRS;
+			ll_opts.variant = XDL_MERGE_FAVOR_THEIRS;
 			break;
 		default:
-			favor = 0;
+			ll_opts.variant = 0;
 			break;
 		}
 	}
@@ -646,10 +649,7 @@ static int merge_3way(struct merge_options *o,
 	read_mmblob(&src2, b->sha1);
 
 	merge_status = ll_merge(result_buf, a->path, &orig, base_name,
-				&src1, name1, &src2, name2,
-				((o->call_depth ? LL_OPT_VIRTUAL_ANCESTOR : 0) |
-				 (o->renormalize ? LL_OPT_RENORMALIZE : 0) |
-				 create_ll_flag(favor)));
+				&src1, name1, &src2, name2, &ll_opts);
 
 	free(name1);
 	free(name2);
diff --git a/rerere.c b/rerere.c
index e40af0d..b180218 100644
--- a/rerere.c
+++ b/rerere.c
@@ -325,7 +325,7 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 	 */
 	ll_merge(&result, path, &mmfile[0], NULL,
 		 &mmfile[1], "ours",
-		 &mmfile[2], "theirs", 0);
+		 &mmfile[2], "theirs", NULL);
 	for (i = 0; i < 3; i++)
 		free(mmfile[i].ptr);
 
-- 
1.7.2.2

[RFC/PATCH jn/merge-renormalize] merge-recursive: expose merge options for builtin merge

[Jonathan Nieder]

There are two very similar blocks of code that recognize options for
the "recursive" merge strategy.  Unify them.

No functional change intended.

Cc: Avery Pennarun <apenwarr@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:
> Justin Frankel wrote:

>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -647,6 +648,14 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
>>  				o.renormalize = 1;
>>  			else if (!strcmp(xopts[x], "no-renormalize"))
>>  				o.renormalize = 0;
>> +			else if (!strcmp(xopts[x], "ignore-all-space"))
>> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE;
[...]
> It's tempting to fix this code duplication once and for all.

 builtin/merge-recursive.c |   14 +-------------
 builtin/merge.c           |   20 ++------------------
 merge-recursive.c         |   21 +++++++++++++++++++++
 merge-recursive.h         |    2 ++
 4 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index c2d4677..a610b68 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -37,19 +37,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 		if (!prefixcmp(arg, "--")) {
 			if (!arg[2])
 				break;
-			if (!strcmp(arg+2, "ours"))
-				o.recursive_variant = MERGE_RECURSIVE_OURS;
-			else if (!strcmp(arg+2, "theirs"))
-				o.recursive_variant = MERGE_RECURSIVE_THEIRS;
-			else if (!strcmp(arg+2, "subtree"))
-				o.subtree_shift = "";
-			else if (!prefixcmp(arg+2, "subtree="))
-				o.subtree_shift = arg + 10;
-			else if (!strcmp(arg+2, "renormalize"))
-				o.renormalize = 1;
-			else if (!strcmp(arg+2, "no-renormalize"))
-				o.renormalize = 0;
-			else
+			if (parse_merge_opt(&o, arg + 2) <= 0)
 				die("Unknown option %s", arg);
 			continue;
 		}
diff --git a/builtin/merge.c b/builtin/merge.c
index 037cd47..8dd81bf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -629,25 +629,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 
 		o.renormalize = option_renormalize;
 
-		/*
-		 * NEEDSWORK: merge with table in builtin/merge-recursive
-		 */
-		for (x = 0; x < xopts_nr; x++) {
-			if (!strcmp(xopts[x], "ours"))
-				o.recursive_variant = MERGE_RECURSIVE_OURS;
-			else if (!strcmp(xopts[x], "theirs"))
-				o.recursive_variant = MERGE_RECURSIVE_THEIRS;
-			else if (!strcmp(xopts[x], "subtree"))
-				o.subtree_shift = "";
-			else if (!prefixcmp(xopts[x], "subtree="))
-				o.subtree_shift = xopts[x]+8;
-			else if (!strcmp(xopts[x], "renormalize"))
-				o.renormalize = 1;
-			else if (!strcmp(xopts[x], "no-renormalize"))
-				o.renormalize = 0;
-			else
+		for (x = 0; x < xopts_nr; x++)
+			if (parse_merge_opt(&o, xopts[x]) <= 0)
 				die("Unknown option for merge-recursive: -X%s", xopts[x]);
-		}
 
 		o.branch1 = head_arg;
 		o.branch2 = remoteheads->item->util;
diff --git a/merge-recursive.c b/merge-recursive.c
index ee52581..063d623 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1499,3 +1499,24 @@ void init_merge_options(struct merge_options *o)
 	memset(&o->current_directory_set, 0, sizeof(struct string_list));
 	o->current_directory_set.strdup_strings = 1;
 }
+
+int parse_merge_opt(struct merge_options *o, const char *s)
+{
+	if (!s || !*s)
+		return 0;
+	if (!strcmp(s, "ours"))
+		o->recursive_variant = MERGE_RECURSIVE_OURS;
+	else if (!strcmp(s, "theirs"))
+		o->recursive_variant = MERGE_RECURSIVE_THEIRS;
+	else if (!strcmp(s, "subtree"))
+		o->subtree_shift = "";
+	else if (!prefixcmp(s, "subtree="))
+		o->subtree_shift = s + strlen("subtree=");
+	else if (!strcmp(s, "renormalize"))
+		o->renormalize = 1;
+	else if (!strcmp(s, "no-renormalize"))
+		o->renormalize = 0;
+	else
+		return 0;
+	return 1;
+}
diff --git a/merge-recursive.h b/merge-recursive.h
index c5fbe79..37ff99a 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -55,6 +55,8 @@ int merge_recursive_generic(struct merge_options *o,
 void init_merge_options(struct merge_options *o);
 struct tree *write_tree_from_memory(struct merge_options *o);
 
+int parse_merge_opt(struct merge_options *out, const char *s);
+
 /* builtin/merge.c */
 int try_merge_command(const char *strategy, struct commit_list *common, const char *head_arg, struct commit_list *remotes);
 
-- 
1.7.2.2

Re: [RFC/PATCH jn/merge-renormalize] merge-recursive: expose merge options for builtin merge

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> There are two very similar blocks of code that recognize options for
> the "recursive" merge strategy.  Unify them.
>
> No functional change intended.

It makes sense, but I wonder why you chose to do "if (fun() <= 0) error"
instead of usual "if (fun() < 0) error"...

Re: [RFC/PATCH jn/merge-renormalize] merge-recursive: expose merge options for builtin merge

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> There are two very similar blocks of code that recognize options for
>> the "recursive" merge strategy.  Unify them.
>>
>> No functional change intended.
>
> It makes sense, but I wonder why you chose to do "if (fun() <= 0) error"
> instead of usual "if (fun() < 0) error"...

No good reason (an attempt to vaguely imitate the interface of
handle_revision_opt() from revision.c).  Since both callers die for
an unrecognized option, returning 0 for success and -1 for failure
would indeed be simpler.

I'll try to find time to reroll the series (including --patience)
on top of jn/merge-renormalize tomorrow.  Probably
--ignore-space-change et al are safe, too, though some test cases
would be nice.

Re: [PATCH v2] git-merge: ignore space support

[Justin Frankel]

On Aug 23, 2010, at 7:28 PM, Jonathan Nieder wrote:

> Justin Frankel wrote:
>
>> This is for the next/ branch
>
> Applies on top of jn/merge-renormalize.  Thanks for the reroll.
>
>> --- a/builtin/merge-recursive.c
>> +++ b/builtin/merge-recursive.c
>> @@ -49,6 +50,14 @@ int cmd_merge_recursive(int argc, const char  
>> **argv, const char *prefix)
>> 				o.renormalize = 1;
>> 			else if (!strcmp(arg+2, "no-renormalize"))
>> 				o.renormalize = 0;
>> +			else if (!strcmp(arg+2, "ignore-all-space"))
>> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE;
>> +			else if (!strcmp(arg+2, "ignore-space-change"))
>> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
>> +			else if (!strcmp(arg+2, "ignore-space-at-eol"))
>> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
>> +			else if (!strcmp(arg+2, "patience"))
>> +				o.xdl_opts |= XDF_PATIENCE_DIFF;
>
> So this teaches merge-recursive and ll-merge to accept the xdiff
> whitespace flags (plus "patience").
>
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -647,6 +648,14 @@ static int try_merge_strategy(const char  
>> *strategy, struct commit_list *common,
>> 				o.renormalize = 1;
>> 			else if (!strcmp(xopts[x], "no-renormalize"))
>> 				o.renormalize = 0;
>> +			else if (!strcmp(xopts[x], "ignore-all-space"))
>> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE;
>> +			else if (!strcmp(xopts[x], "ignore-space-change"))
>> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
>> +			else if (!strcmp(xopts[x], "ignore-space-at-eol"))
>> +				o.xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
>> +			else if (!strcmp(xopts[x], "patience"))
>> +				o.xdl_opts |= XDF_PATIENCE_DIFF;
>
> It's tempting to fix this code duplication once and for all.
>
I was tempted to do that, too, but felt I was out of my element.

>> +++ b/ll-merge.h
>> @@ -7,19 +7,32 @@
>>
>> #define LL_OPT_VIRTUAL_ANCESTOR	(1 << 0)
>> #define LL_OPT_FAVOR_MASK	((1 << 1) | (1 << 2))
>> -#define LL_OPT_FAVOR_SHIFT 1
>> +#define LL_OPT_FAVOR_SHIFT 	1
>> #define LL_OPT_RENORMALIZE	(1 << 3)
>> +#define LL_OPT_XDL_MASK 	(0x3F << 4)
>> +#define LL_OPT_XDL_SHIFT 	4
>
> The space-instead-of-tab was intentional: the idea is that this is a
> table:
>
> 	name			bits
> 	VIRTUAL_ANCESTOR	0
> 	FAVOR_MASK		1, 2
> 	FAVOR_SHIFT = 1
> 	RENORMALIZE		3
> 	WHITESPACE_MASK		4, 5, 6
> 	WHITESPACE_SHIFT = 4
> 	PATIENCE		7
>
> and since a shift is not a bitmask it does not deserve an entry in the
> table.
>
Ah.. this gets a bit tedious if you have a 5 bit flag though, no? And  
since the XDL options are all defined internally by xdiff, perhaps it  
makes sense to abstract out whitespace/patience, and just pass that  
flag around as an opaque XDL-flag.

> The flag layout worries me a bit because presumably the next extension
> would get bit 8, and so on.  What happens if xdiff learns another
> whitespace-ignoring mode?

How about adding a define in xdiff.h for XDF_DIFF_FLAG_BITS , which  
defines the number of bits relevant, build the mask/bits based on  
this... the nice thing is that all of the bits are defined in one  
place, so no other code would need to be modified. Having said that,  
the solution you pasted below (struct instead of flag) is probably the  
cleanest so this paragraph is moot. Sorry for wasting the bits.

>
>>
>> static inline int ll_opt_favor(int flag)
>> {
>> 	return (flag & LL_OPT_FAVOR_MASK) >> LL_OPT_FAVOR_SHIFT;
>> }
>>
>> +static inline int ll_opt_xdl_opt(int flag)
>> +{
>> +	return ((flag & LL_OPT_XDL_MASK) >> LL_OPT_XDL_SHIFT);
>> +}
>
> Style nit: parentheses around a return value are not needed.

Yeah, I was imitating the other function ..

>
>> static inline int create_ll_flag(int favor)
>> {
>> 	return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK);
>> }
>
> Looks like I am guilty of the same thing.
>
>> +static inline int create_ll_flag_xdl_opt(int favor, int xdl_opt)
>> +{
>> +	return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK) |
>> +		((xdl_opt << LL_OPT_XDL_SHIFT) & LL_OPT_XDL_MASK);
>> +}
>
> This function (like its cousin create_ll_flag) is ugly, as Bert
> noticed before.  I am starting to wonder if it would not be simpler
> to use
>
> extern int ll_merge(..., const struct ll_merge_options *opts);
>
> treating NULL as a request for the default options.
>
> In other words, how about something like this?
>
Fine by me... (ok if I were to nitpick I would probably make most of  
the internal static functions check that opts was non-NULL before  
dereferencing, in case the calling code ever changed, but I'm the noob).

Thanks for looking at my patch ;)

-Justin

> -- 8< --
> Subject: ll-merge: replace flag argument with options struct
>
> Keeping track of the flag bits is proving more trouble than it's
> worth.  Instead, use a pointer to an options struct like most similar
> APIs do.
>
> Callers with no special requests can pass NULL to request the default
> options.
>
> Cc: Bert Wesarg <bert.wesarg@googlemail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> builtin/checkout.c |    2 +-
> ll-merge.c         |   42 ++++++++++++++++++++++++++----------------
> ll-merge.h         |   21 ++++++---------------
> merge-file.c       |    2 +-
> merge-recursive.c  |   22 +++++++++++-----------
> rerere.c           |    2 +-
> 6 files changed, 46 insertions(+), 45 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 24b67d5..4d36b28 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -155,7 +155,7 @@ static int checkout_merged(int pos, struct  
> checkout *state)
> 	 * merge.renormalize set, too
> 	 */
> 	status = ll_merge(&result_buf, path, &ancestor, "base",
> -			  &ours, "ours", &theirs, "theirs", 0);
> +			  &ours, "ours", &theirs, "theirs", NULL);
> 	free(ancestor.ptr);
> 	free(ours.ptr);
> 	free(theirs.ptr);
> diff --git a/ll-merge.c b/ll-merge.c
> index 6bb3095..98f353a 100644
> --- a/ll-merge.c
> +++ b/ll-merge.c
> @@ -18,7 +18,7 @@ typedef int (*ll_merge_fn)(const struct  
> ll_merge_driver *,
> 			   mmfile_t *orig, const char *orig_name,
> 			   mmfile_t *src1, const char *name1,
> 			   mmfile_t *src2, const char *name2,
> -			   int flag,
> +			   const struct ll_merge_options *opts,
> 			   int marker_size);
>
> struct ll_merge_driver {
> @@ -39,14 +39,15 @@ static int ll_binary_merge(const struct  
> ll_merge_driver *drv_unused,
> 			   mmfile_t *orig, const char *orig_name,
> 			   mmfile_t *src1, const char *name1,
> 			   mmfile_t *src2, const char *name2,
> -			   int flag, int marker_size)
> +			   const struct ll_merge_options *opts,
> +			   int marker_size)
> {
> 	/*
> 	 * The tentative merge result is "ours" for the final round,
> 	 * or common ancestor for an internal merge.  Still return
> 	 * "conflicted merge" status.
> 	 */
> -	mmfile_t *stolen = (flag & LL_OPT_VIRTUAL_ANCESTOR) ? orig : src1;
> +	mmfile_t *stolen = opts->virtual_ancestor ? orig : src1;
>
> 	result->ptr = stolen->ptr;
> 	result->size = stolen->size;
> @@ -60,7 +61,8 @@ static int ll_xdl_merge(const struct  
> ll_merge_driver *drv_unused,
> 			mmfile_t *orig, const char *orig_name,
> 			mmfile_t *src1, const char *name1,
> 			mmfile_t *src2, const char *name2,
> -			int flag, int marker_size)
> +			const struct ll_merge_options *opts,
> +			int marker_size)
> {
> 	xmparam_t xmp;
>
> @@ -74,12 +76,12 @@ static int ll_xdl_merge(const struct  
> ll_merge_driver *drv_unused,
> 				       orig, orig_name,
> 				       src1, name1,
> 				       src2, name2,
> -				       flag, marker_size);
> +				       opts, marker_size);
> 	}
>
> 	memset(&xmp, 0, sizeof(xmp));
> 	xmp.level = XDL_MERGE_ZEALOUS;
> -	xmp.favor = ll_opt_favor(flag);
> +	xmp.favor = opts->variant;
> 	if (git_xmerge_style >= 0)
> 		xmp.style = git_xmerge_style;
> 	if (marker_size > 0)
> @@ -96,14 +98,15 @@ static int ll_union_merge(const struct  
> ll_merge_driver *drv_unused,
> 			  mmfile_t *orig, const char *orig_name,
> 			  mmfile_t *src1, const char *name1,
> 			  mmfile_t *src2, const char *name2,
> -			  int flag, int marker_size)
> +			  const struct ll_merge_options *opts,
> +			  int marker_size)
> {
> 	/* Use union favor */
> -	flag &= ~LL_OPT_FAVOR_MASK;
> -	flag |= create_ll_flag(XDL_MERGE_FAVOR_UNION);
> +	struct ll_merge_options o = *opts;
> +	o.variant = XDL_MERGE_FAVOR_UNION;
> 	return ll_xdl_merge(drv_unused, result, path_unused,
> 			    orig, NULL, src1, NULL, src2, NULL,
> -			    flag, marker_size);
> +			    &o, marker_size);
> 	return 0;
> }
>
> @@ -136,7 +139,8 @@ static int ll_ext_merge(const struct  
> ll_merge_driver *fn,
> 			mmfile_t *orig, const char *orig_name,
> 			mmfile_t *src1, const char *name1,
> 			mmfile_t *src2, const char *name2,
> -			int flag, int marker_size)
> +			const struct ll_merge_options *opts,
> +			int marker_size)
> {
> 	char temp[4][50];
> 	struct strbuf cmd = STRBUF_INIT;
> @@ -337,15 +341,21 @@ int ll_merge(mmbuffer_t *result_buf,
> 	     mmfile_t *ancestor, const char *ancestor_label,
> 	     mmfile_t *ours, const char *our_label,
> 	     mmfile_t *theirs, const char *their_label,
> -	     int flag)
> +	     const struct ll_merge_options *opts)
> {
> 	static struct git_attr_check check[2];
> 	const char *ll_driver_name = NULL;
> 	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> 	const struct ll_merge_driver *driver;
> -	int virtual_ancestor = flag & LL_OPT_VIRTUAL_ANCESTOR;
>
> -	if (flag & LL_OPT_RENORMALIZE) {
> +	if (!opts) {
> +		struct ll_merge_options default_opts = {0};
> +		return ll_merge(result_buf, path, ancestor, ancestor_label,
> +				ours, our_label, theirs, their_label,
> +				&default_opts);
> +	}
> +
> +	if (opts->renormalize) {
> 		normalize_file(ancestor, path);
> 		normalize_file(ours, path);
> 		normalize_file(theirs, path);
> @@ -359,11 +369,11 @@ int ll_merge(mmbuffer_t *result_buf,
> 		}
> 	}
> 	driver = find_ll_merge_driver(ll_driver_name);
> -	if (virtual_ancestor && driver->recursive)
> +	if (opts->virtual_ancestor && driver->recursive)
> 		driver = find_ll_merge_driver(driver->recursive);
> 	return driver->fn(driver, result_buf, path, ancestor, ancestor_label,
> 			  ours, our_label, theirs, their_label,
> -			  flag, marker_size);
> +			  opts, marker_size);
> }
>
> int ll_merge_marker_size(const char *path)
> diff --git a/ll-merge.h b/ll-merge.h
> index ff7ca87..4b707f0 100644
> --- a/ll-merge.h
> +++ b/ll-merge.h
> @@ -5,27 +5,18 @@
> #ifndef LL_MERGE_H
> #define LL_MERGE_H
>
> -#define LL_OPT_VIRTUAL_ANCESTOR	(1 << 0)
> -#define LL_OPT_FAVOR_MASK	((1 << 1) | (1 << 2))
> -#define LL_OPT_FAVOR_SHIFT 1
> -#define LL_OPT_RENORMALIZE	(1 << 3)
> -
> -static inline int ll_opt_favor(int flag)
> -{
> -	return (flag & LL_OPT_FAVOR_MASK) >> LL_OPT_FAVOR_SHIFT;
> -}
> -
> -static inline int create_ll_flag(int favor)
> -{
> -	return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK);
> -}
> +struct ll_merge_options {
> +	unsigned virtual_ancestor : 1;
> +	unsigned variant : 2;	/* favor ours, favor theirs, or union merge */
> +	unsigned renormalize : 1;
> +};
>
> int ll_merge(mmbuffer_t *result_buf,
> 	     const char *path,
> 	     mmfile_t *ancestor, const char *ancestor_label,
> 	     mmfile_t *ours, const char *our_label,
> 	     mmfile_t *theirs, const char *their_label,
> -	     int flag);
> +	     const struct ll_merge_options *opts);
>
> int ll_merge_marker_size(const char *path);
>
> diff --git a/merge-file.c b/merge-file.c
> index db4d0d5..f7f4533 100644
> --- a/merge-file.c
> +++ b/merge-file.c
> @@ -37,7 +37,7 @@ static void *three_way_filemerge(const char *path,  
> mmfile_t *base, mmfile_t *our
> 	 * common ancestor.
> 	 */
> 	merge_status = ll_merge(&res, path, base, NULL,
> -				our, ".our", their, ".their", 0);
> +				our, ".our", their, ".their", NULL);
> 	if (merge_status < 0)
> 		return NULL;
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 762b549..ed09785 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -608,22 +608,25 @@ static int merge_3way(struct merge_options *o,
> 		      const char *branch2)
> {
> 	mmfile_t orig, src1, src2;
> +	struct ll_merge_options ll_opts = {0};
> 	char *base_name, *name1, *name2;
> 	int merge_status;
> -	int favor;
>
> -	if (o->call_depth)
> -		favor = 0;
> -	else {
> +	ll_opts.renormalize = o->renormalize;
> +
> +	if (o->call_depth) {
> +		ll_opts.virtual_ancestor = 1;
> +		ll_opts.variant = 0;
> +	} else {
> 		switch (o->recursive_variant) {
> 		case MERGE_RECURSIVE_OURS:
> -			favor = XDL_MERGE_FAVOR_OURS;
> +			ll_opts.variant = XDL_MERGE_FAVOR_OURS;
> 			break;
> 		case MERGE_RECURSIVE_THEIRS:
> -			favor = XDL_MERGE_FAVOR_THEIRS;
> +			ll_opts.variant = XDL_MERGE_FAVOR_THEIRS;
> 			break;
> 		default:
> -			favor = 0;
> +			ll_opts.variant = 0;
> 			break;
> 		}
> 	}
> @@ -646,10 +649,7 @@ static int merge_3way(struct merge_options *o,
> 	read_mmblob(&src2, b->sha1);
>
> 	merge_status = ll_merge(result_buf, a->path, &orig, base_name,
> -				&src1, name1, &src2, name2,
> -				((o->call_depth ? LL_OPT_VIRTUAL_ANCESTOR : 0) |
> -				 (o->renormalize ? LL_OPT_RENORMALIZE : 0) |
> -				 create_ll_flag(favor)));
> +				&src1, name1, &src2, name2, &ll_opts);
>
> 	free(name1);
> 	free(name2);
> diff --git a/rerere.c b/rerere.c
> index e40af0d..b180218 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -325,7 +325,7 @@ static int handle_cache(const char *path,  
> unsigned char *sha1, const char *outpu
> 	 */
> 	ll_merge(&result, path, &mmfile[0], NULL,
> 		 &mmfile[1], "ours",
> -		 &mmfile[2], "theirs", 0);
> +		 &mmfile[2], "theirs", NULL);
> 	for (i = 0; i < 3; i++)
> 		free(mmfile[i].ptr);
>
> -- 
> 1.7.2.2

Re: [PATCH v2] git-merge: ignore space support

[Jonathan Nieder]

Justin Frankel wrote:

> Fine by me... (ok if I were to nitpick I would probably make most of
> the internal static functions check that opts was non-NULL before
> dereferencing, in case the calling code ever changed

Not a bad idea.  I'll squash in something like this.

A real BUG_ON macro would make this less ugly.

-- 8< --
Subject: ll-merge: BUG_ON(!opts) in internal functions

If one of the functions used to implement ll_merge() gets exposed,
callers would be likely to pass a NULL options argument, resulting in
segfaults.  Die with a more meaningful message in that case.

Suggested-by: Justin Frankel <justin@cockos.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 ll-merge.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/ll-merge.c b/ll-merge.c
index 98f353a..8ff0b27 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -42,12 +42,17 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 			   const struct ll_merge_options *opts,
 			   int marker_size)
 {
+	mmfile_t *stolen;
+
+	if (!opts)
+		die("BUG: !opts in binary merge driver");
+
 	/*
 	 * The tentative merge result is "ours" for the final round,
 	 * or common ancestor for an internal merge.  Still return
 	 * "conflicted merge" status.
 	 */
-	mmfile_t *stolen = opts->virtual_ancestor ? orig : src1;
+	stolen = opts->virtual_ancestor ? orig : src1;
 
 	result->ptr = stolen->ptr;
 	result->size = stolen->size;
@@ -66,6 +71,9 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 {
 	xmparam_t xmp;
 
+	if (!opts)
+		die("BUG: !opts in xdiff merge driver");
+
 	if (buffer_is_binary(orig->ptr, orig->size) ||
 	    buffer_is_binary(src1->ptr, src1->size) ||
 	    buffer_is_binary(src2->ptr, src2->size)) {
@@ -102,7 +110,10 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,
 			  int marker_size)
 {
 	/* Use union favor */
-	struct ll_merge_options o = *opts;
+	struct ll_merge_options o;
+	if (!opts)
+		die("BUG: !opts in union merge driver");
+	o = *opts;
 	o.variant = XDL_MERGE_FAVOR_UNION;
 	return ll_xdl_merge(drv_unused, result, path_unused,
 			    orig, NULL, src1, NULL, src2, NULL,
@@ -149,6 +160,9 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 	int status, fd, i;
 	struct stat st;
 
+	if (!opts)
+		die("BUG: !opts in custom merge driver");
+
 	dict[0].placeholder = "O"; dict[0].value = temp[0];
 	dict[1].placeholder = "A"; dict[1].value = temp[1];
 	dict[2].placeholder = "B"; dict[2].value = temp[2];
-- 
1.7.2.2

Re: [PATCH v2] git-merge: ignore space support

[Bert Wesarg]

Hi,

On Wed, Aug 25, 2010 at 06:40, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Justin Frankel wrote:
>
>> Fine by me... (ok if I were to nitpick I would probably make most of
>> the internal static functions check that opts was non-NULL before
>> dereferencing, in case the calling code ever changed
>
> Not a bad idea.  I'll squash in something like this.
>
> A real BUG_ON macro would make this less ugly.

I think exactly because of this there is no prior art of using NULL as
'use the default options'. Without this all these NULL pointer checks
wont be necessary.

Bert

Re: [PATCH v2] git-merge: ignore space support

[Justin Frankel]

Bert Wesarg wrote:
> Hi,
>
> On Wed, Aug 25, 2010 at 06:40, Jonathan Nieder <jrnieder@gmail.com> wrote:
>   
>> Justin Frankel wrote:
>>
>>     
>>> Fine by me... (ok if I were to nitpick I would probably make most of
>>> the internal static functions check that opts was non-NULL before
>>> dereferencing, in case the calling code ever changed
>>>       
>> Not a bad idea.  I'll squash in something like this.
>>
>> A real BUG_ON macro would make this less ugly.
>>     
>
> I think exactly because of this there is no prior art of using NULL as
> 'use the default options'. Without this all these NULL pointer checks
> wont be necessary.
>   

The only danger is that ll_merge()'s signature didn't change in such a 
way to break compilation, i.e:

int ll_merge(mmbuffer_t *result_buf,
             const char *path,
             mmfile_t *ancestor, const char *ancestor_label,
             mmfile_t *ours, const char *our_label,
             mmfile_t *theirs, const char *their_label,
             int flag);

becomes:

int ll_merge(mmbuffer_t *result_buf,
             const char *path,
             mmfile_t *ancestor, const char *ancestor_label,
             mmfile_t *ours, const char *our_label,
             mmfile_t *theirs, const char *their_label,
             struct whatever *conf);

In this case, passing 0 as the last parameter will compile either way.

Sure, we can grep all of the source, but who knows when something else 
will get merged in...

-Justin

Re: [PATCH v2] git-merge: ignore space support

[Junio C Hamano]

Justin Frankel <justin@cockos.com> writes:

> The only danger is that ll_merge()'s signature didn't change in such a
> way to break compilation, i.e:
>
> int ll_merge(mmbuffer_t *result_buf,
>             const char *path,
>             mmfile_t *ancestor, const char *ancestor_label,
>             mmfile_t *ours, const char *our_label,
>             mmfile_t *theirs, const char *their_label,
>             int flag);
>
> becomes:
>
> int ll_merge(mmbuffer_t *result_buf,
>             const char *path,
>             mmfile_t *ancestor, const char *ancestor_label,
>             mmfile_t *ours, const char *our_label,
>             mmfile_t *theirs, const char *their_label,
>             struct whatever *conf);
>
> In this case, passing 0 as the last parameter will compile either way.
>
> Sure, we can grep all of the source, but who knows when something else
> will get merged in...

That is technically a valid concern but I suspect it does not matter in
this particular case, where integer 0 used to mean "use the default" and
the new API uses NULL to mean the same.

If an existing call site used to pass 0 and the patch forgot to update it,
it will look ugly (we encourage to spell a NULL pointer "NULL", not "0",
in our codebase) but no harm is done.  If an existing call site asked for
a non-default behaviour by passing a non-zero integer flag, and the patch
forgot to update it, the compiler would have caught it.  Merging a side
branch is the same deal; if it adds a call with a non-zero argument to ask
for a non-default behaviour, that will be done via an expression over some
integer variables or constants, and that won't be casted silently to a
pointer to "struct whatever", no?

Re: [PATCH v2] git-merge: ignore space support

[Justin Frankel]

Junio C Hamano wrote:
> Justin Frankel <justin@cockos.com> writes:
>
>   
>> The only danger is that ll_merge()'s signature didn't change in such a
>> way to break compilation, i.e:
>>
>> int ll_merge(mmbuffer_t *result_buf,
>>             const char *path,
>>             mmfile_t *ancestor, const char *ancestor_label,
>>             mmfile_t *ours, const char *our_label,
>>             mmfile_t *theirs, const char *their_label,
>>             int flag);
>>
>> becomes:
>>
>> int ll_merge(mmbuffer_t *result_buf,
>>             const char *path,
>>             mmfile_t *ancestor, const char *ancestor_label,
>>             mmfile_t *ours, const char *our_label,
>>             mmfile_t *theirs, const char *their_label,
>>             struct whatever *conf);
>>
>> In this case, passing 0 as the last parameter will compile either way.
>>
>> Sure, we can grep all of the source, but who knows when something else
>> will get merged in...
>>     
>
> That is technically a valid concern but I suspect it does not matter in
> this particular case, where integer 0 used to mean "use the default" and
> the new API uses NULL to mean the same.
>
> If an existing call site used to pass 0 and the patch forgot to update it,
> it will look ugly (we encourage to spell a NULL pointer "NULL", not "0",
> in our codebase) but no harm is done.  If an existing call site asked for
> a non-default behaviour by passing a non-zero integer flag, and the patch
> forgot to update it, the compiler would have caught it.  Merging a side
> branch is the same deal; if it adds a call with a non-zero argument to ask
> for a non-default behaviour, that will be done via an expression over some
> integer variables or constants, and that won't be casted silently to a
> pointer to "struct whatever", no?
>
>   
Agreed, I was responding to Bert's email, in which he stated that he 
hadn't seen NULL-for-default anywhere else in git. Using NULL for 
default behavior is good in that it handles un-updated code and merges 
correctly (passing 0 uses defaults, passing nonzero fails compile).


> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH v2] git-merge: ignore space support

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The flag layout worries me a bit because presumably the next extension
> would get bit 8, and so on.  What happens if xdiff learns another
> whitespace-ignoring mode?
> ...
> This function (like its cousin create_ll_flag) is ugly, as Bert
> noticed before.  I am starting to wonder if it would not be simpler
> to use
>
>  extern int ll_merge(..., const struct ll_merge_options *opts);
>
> treating NULL as a request for the default options.
>
> In other words, how about something like this?

Sounds like a good direction to go.

Re: [PATCH v2] git-merge: ignore space support

[Bert Wesarg]

Hi,

On Tue, Aug 24, 2010 at 04:28, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Subject: ll-merge: replace flag argument with options struct
>
> Keeping track of the flag bits is proving more trouble than it's
> worth.  Instead, use a pointer to an options struct like most similar
> APIs do.
>
> Callers with no special requests can pass NULL to request the default
> options.

thanks for doing this. But is there any prior art for using NULL as
'use the default flags' in the project? For what I know, I can't think
of an example. But my inside knowledge into git drifts slowly away.

Regards,
Bert

>
> Cc: Bert Wesarg <bert.wesarg@googlemail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v2] git-merge: ignore space support

[Jonathan Nieder]

Hi,

Bert Wesarg wrote:

> thanks for doing this. But is there any prior art for using NULL as
> 'use the default flags' in the project? For what I know, I can't think
> of an example. But my inside knowledge into git drifts slowly away.

Not that I know of for option flags.  Probably worth mentioning in the
API docs, which I conveniently forgot to update.

-- 8< --
Subject: ll-merge: document options struct

In particular, try to make it clear that most callers will want to
pass `NULL` as the `opts` parameter.

Cc: Bert Wesarg <bert.wesarg@googlemail.com>
Cc: Avery Pennarun <apenwarr@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff --git a/Documentation/technical/api-merge.txt b/Documentation/technical/api-merge.txt
index a7e050b..9dc1bed 100644
--- a/Documentation/technical/api-merge.txt
+++ b/Documentation/technical/api-merge.txt
@@ -17,6 +17,40 @@ responsible for a few things.
    path-specific merge drivers (specified in `.gitattributes`)
    into account.
 
+Data structures
+---------------
+
+* `mmbuffer_t`, `mmfile_t`
+
+These store data usable for use by the xdiff backend, for writing and
+for reading, respectively.  See `xdiff/xdiff.h` for the definitions
+and `diff.c` for examples.
+
+* `struct ll_merge_options`
+
+This describes the set of options the calling program wants to affect
+the operation of a low-level (single file) merge.  Some options:
+
+`virtual_ancestor`::
+	Behave as though this were part of a merge between common
+	ancestors in a recursive merge.
+	If a helper program is specified by the
+	`[merge "<driver>"] recursive` configuration, it will
+	be used (see linkgit:gitattributes[5]).
+
+`variant`::
+	Resolve local conflicts automatically in favor
+	of one side or the other (as in 'git merge-file'
+	`--ours`/`--theirs`/`--union`).  Can be `0`,
+	`XDL_MERGE_FAVOR_OURS`, `XDL_MERGE_FAVOR_THEIRS`, or
+	`XDL_MERGE_FAVOR_UNION`.
+
+`renormalize`::
+	Resmudge and clean the "base", "theirs" and "ours" files
+	before merging.  Use this when the merge is likely to have
+	overlapped with a change in smudge/clean or end-of-line
+	normalization rules.
+
 Low-level (single file) merge
 -----------------------------
 
@@ -28,15 +62,24 @@ Low-level (single file) merge
 	`.git/info/attributes` into account.  Returns 0 for a
 	clean merge.
 
-The caller:
+Calling sequence:
 
-1. allocates an mmbuffer_t variable for the result;
-2. allocates and fills variables with the file's original content
-   and two modified versions (using `read_mmfile`, for example);
-3. calls ll_merge();
-4. reads the output from result_buf.ptr and result_buf.size;
-5. releases buffers when finished (free(ancestor.ptr); free(ours.ptr);
-   free(theirs.ptr); free(result_buf.ptr);).
+* Prepare a `struct ll_merge_options` to record options.
+  If you have no special requests, skip this and pass `NULL`
+  as the `opts` parameter to use the default options.
+
+* Allocate an mmbuffer_t variable for the result.
+
+* Allocate and fill variables with the file's original content
+  and two modified versions (using `read_mmfile`, for example).
+
+* Call `ll_merge()`.
+
+* Read the merged content from `result_buf.ptr` and `result_buf.size`.
+
+* Release buffers when finished.  A simple
+  `free(ancestor.ptr); free(ours.ptr); free(theirs.ptr);
+  free(result_buf.ptr);` will do.
 
 If the modifications do not merge cleanly, `ll_merge` will return a
 nonzero value and `result_buf` will generally include a description of
@@ -47,18 +90,6 @@ The `ancestor_label`, `our_label`, and `their_label` parameters are
 used to label the different sides of a conflict if the merge driver
 supports this.
 
-The `flag` parameter is a bitfield:
-
- - The `LL_OPT_VIRTUAL_ANCESTOR` bit indicates whether this is an
-   internal merge to consolidate ancestors for a recursive merge.
-
- - The `LL_OPT_FAVOR_MASK` bits allow local conflicts to be automatically
-   resolved in favor of one side or the other (as in 'git merge-file'
-   `--ours`/`--theirs`/`--union`).
-   They can be populated by `create_ll_flag`, whose argument can be
-   `XDL_MERGE_FAVOR_OURS`, `XDL_MERGE_FAVOR_THEIRS`, or
-   `XDL_MERGE_FAVOR_UNION`.
-
 Everything else
 ---------------
 
-- 

[PATCH v3 0/4] git-merge: ignore space support

[Jonathan Nieder]

Hi,

Justin Frankel wrote:

> git-merge and git-rebase can now be passed -Xignore-space-at-eol,
> -Xignore-space-change, -Xignore-all-space, -Xpatience options.

I've tried this out a little and it seems to do the right thing.
Here's the series in its current state.

Patch 1 is to avoid the code duplication between builtin/merge and
merge-recursive you noticed, by adding a function that recognizes -X
options to merge-recursive.h.

Unlike the trial balloon patch I sent before, it uses a conventional
return value with 0 meaning "recognized" and -1 meaning "not recognized".

Patch 2 replaces the flag parameter to ll_merge with a struct.
I think the maintenance trouble of keeping track of the flag bits is
not worth it for a marginal speedup.

Patches 3 and 4 provide "merge -Xpatience" and
"merge -Xignore-space-change" et al on top of this foundation.
I split them up and put patience first because it is worth thinking
carefully about which version of a line with only whitespace changes
a merge will take.

No tests yet.  (If someone would like to add them, that would be
great.)  Probably after responding to initial comments I will not have
much time for this series, so please feel free to pick it up and take
it in whatever direction you wish.

Jonathan Nieder (2):
  merge-recursive: expose merge options for builtin merge
  ll-merge: replace flag argument with options struct

Justin Frankel (2):
  merge-recursive --patience
  merge-recursive: options to ignore whitespace changes

 Documentation/merge-strategies.txt    |   22 ++++++++++
 Documentation/technical/api-merge.txt |   73 +++++++++++++++++++++++---------
 builtin/checkout.c                    |    2 +-
 builtin/merge-recursive.c             |   15 +------
 builtin/merge.c                       |   20 +--------
 ll-merge.c                            |   50 +++++++++++++++-------
 ll-merge.h                            |   22 +++-------
 merge-file.c                          |    2 +-
 merge-recursive.c                     |   54 +++++++++++++++++++-----
 merge-recursive.h                     |    3 +
 rerere.c                              |    2 +-
 11 files changed, 168 insertions(+), 97 deletions(-)

-- 
1.7.2.2

[PATCH 1/4] merge-recursive: expose merge options for builtin merge

[Jonathan Nieder]

There are two very similar blocks of code that recognize options for
the "recursive" merge strategy.  Unify them.

No functional change intended.

Cc: Avery Pennarun <apenwarr@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/merge-recursive.c |   14 +-------------
 builtin/merge.c           |   20 ++------------------
 merge-recursive.c         |   21 +++++++++++++++++++++
 merge-recursive.h         |    2 ++
 4 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index c2d4677..5a52f3d 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -37,19 +37,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 		if (!prefixcmp(arg, "--")) {
 			if (!arg[2])
 				break;
-			if (!strcmp(arg+2, "ours"))
-				o.recursive_variant = MERGE_RECURSIVE_OURS;
-			else if (!strcmp(arg+2, "theirs"))
-				o.recursive_variant = MERGE_RECURSIVE_THEIRS;
-			else if (!strcmp(arg+2, "subtree"))
-				o.subtree_shift = "";
-			else if (!prefixcmp(arg+2, "subtree="))
-				o.subtree_shift = arg + 10;
-			else if (!strcmp(arg+2, "renormalize"))
-				o.renormalize = 1;
-			else if (!strcmp(arg+2, "no-renormalize"))
-				o.renormalize = 0;
-			else
+			if (parse_merge_opt(&o, arg + 2))
 				die("Unknown option %s", arg);
 			continue;
 		}
diff --git a/builtin/merge.c b/builtin/merge.c
index 037cd47..721c424 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -629,25 +629,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 
 		o.renormalize = option_renormalize;
 
-		/*
-		 * NEEDSWORK: merge with table in builtin/merge-recursive
-		 */
-		for (x = 0; x < xopts_nr; x++) {
-			if (!strcmp(xopts[x], "ours"))
-				o.recursive_variant = MERGE_RECURSIVE_OURS;
-			else if (!strcmp(xopts[x], "theirs"))
-				o.recursive_variant = MERGE_RECURSIVE_THEIRS;
-			else if (!strcmp(xopts[x], "subtree"))
-				o.subtree_shift = "";
-			else if (!prefixcmp(xopts[x], "subtree="))
-				o.subtree_shift = xopts[x]+8;
-			else if (!strcmp(xopts[x], "renormalize"))
-				o.renormalize = 1;
-			else if (!strcmp(xopts[x], "no-renormalize"))
-				o.renormalize = 0;
-			else
+		for (x = 0; x < xopts_nr; x++)
+			if (parse_merge_opt(&o, xopts[x]))
 				die("Unknown option for merge-recursive: -X%s", xopts[x]);
-		}
 
 		o.branch1 = head_arg;
 		o.branch2 = remoteheads->item->util;
diff --git a/merge-recursive.c b/merge-recursive.c
index 762b549..44576b7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1499,3 +1499,24 @@ void init_merge_options(struct merge_options *o)
 	memset(&o->current_directory_set, 0, sizeof(struct string_list));
 	o->current_directory_set.strdup_strings = 1;
 }
+
+int parse_merge_opt(struct merge_options *o, const char *s)
+{
+	if (!s || !*s)
+		return -1;
+	if (!strcmp(s, "ours"))
+		o->recursive_variant = MERGE_RECURSIVE_OURS;
+	else if (!strcmp(s, "theirs"))
+		o->recursive_variant = MERGE_RECURSIVE_THEIRS;
+	else if (!strcmp(s, "subtree"))
+		o->subtree_shift = "";
+	else if (!prefixcmp(s, "subtree="))
+		o->subtree_shift = s + strlen("subtree=");
+	else if (!strcmp(s, "renormalize"))
+		o->renormalize = 1;
+	else if (!strcmp(s, "no-renormalize"))
+		o->renormalize = 0;
+	else
+		return -1;
+	return 0;
+}
diff --git a/merge-recursive.h b/merge-recursive.h
index c5fbe79..37ff99a 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -55,6 +55,8 @@ int merge_recursive_generic(struct merge_options *o,
 void init_merge_options(struct merge_options *o);
 struct tree *write_tree_from_memory(struct merge_options *o);
 
+int parse_merge_opt(struct merge_options *out, const char *s);
+
 /* builtin/merge.c */
 int try_merge_command(const char *strategy, struct commit_list *common, const char *head_arg, struct commit_list *remotes);
 
-- 
1.7.2.2

[PATCH 2/4] ll-merge: replace flag argument with options struct

[Jonathan Nieder]

Keeping track of the flag bits is proving more trouble than it's
worth.  Instead, use a pointer to an options struct like most similar
APIs do.

Callers with no special requests can pass NULL to request the default
options.

Cc: Bert Wesarg <bert.wesarg@googlemail.com>
Cc: Avery Pennarun <apenwarr@gmail.com>
Helped-by: Justin Frankel <justin@cockos.com>
Helped-by: Bert Wesarg <bert.wesarg@googlemail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This time, with updated documentation.

Let's avoid the die("BUG: ...") ugliness by using assert().

 Documentation/technical/api-merge.txt |   73 +++++++++++++++++++++++---------
 builtin/checkout.c                    |    2 +-
 ll-merge.c                            |   49 +++++++++++++++-------
 ll-merge.h                            |   21 +++-------
 merge-file.c                          |    2 +-
 merge-recursive.c                     |   22 +++++-----
 rerere.c                              |    2 +-
 7 files changed, 105 insertions(+), 66 deletions(-)

diff --git a/Documentation/technical/api-merge.txt b/Documentation/technical/api-merge.txt
index a7e050b..9dc1bed 100644
--- a/Documentation/technical/api-merge.txt
+++ b/Documentation/technical/api-merge.txt
@@ -17,6 +17,40 @@ responsible for a few things.
    path-specific merge drivers (specified in `.gitattributes`)
    into account.
 
+Data structures
+---------------
+
+* `mmbuffer_t`, `mmfile_t`
+
+These store data usable for use by the xdiff backend, for writing and
+for reading, respectively.  See `xdiff/xdiff.h` for the definitions
+and `diff.c` for examples.
+
+* `struct ll_merge_options`
+
+This describes the set of options the calling program wants to affect
+the operation of a low-level (single file) merge.  Some options:
+
+`virtual_ancestor`::
+	Behave as though this were part of a merge between common
+	ancestors in a recursive merge.
+	If a helper program is specified by the
+	`[merge "<driver>"] recursive` configuration, it will
+	be used (see linkgit:gitattributes[5]).
+
+`variant`::
+	Resolve local conflicts automatically in favor
+	of one side or the other (as in 'git merge-file'
+	`--ours`/`--theirs`/`--union`).  Can be `0`,
+	`XDL_MERGE_FAVOR_OURS`, `XDL_MERGE_FAVOR_THEIRS`, or
+	`XDL_MERGE_FAVOR_UNION`.
+
+`renormalize`::
+	Resmudge and clean the "base", "theirs" and "ours" files
+	before merging.  Use this when the merge is likely to have
+	overlapped with a change in smudge/clean or end-of-line
+	normalization rules.
+
 Low-level (single file) merge
 -----------------------------
 
@@ -28,15 +62,24 @@ Low-level (single file) merge
 	`.git/info/attributes` into account.  Returns 0 for a
 	clean merge.
 
-The caller:
-
-1. allocates an mmbuffer_t variable for the result;
-2. allocates and fills variables with the file's original content
-   and two modified versions (using `read_mmfile`, for example);
-3. calls ll_merge();
-4. reads the output from result_buf.ptr and result_buf.size;
-5. releases buffers when finished (free(ancestor.ptr); free(ours.ptr);
-   free(theirs.ptr); free(result_buf.ptr);).
+Calling sequence:
+
+* Prepare a `struct ll_merge_options` to record options.
+  If you have no special requests, skip this and pass `NULL`
+  as the `opts` parameter to use the default options.
+
+* Allocate an mmbuffer_t variable for the result.
+
+* Allocate and fill variables with the file's original content
+  and two modified versions (using `read_mmfile`, for example).
+
+* Call `ll_merge()`.
+
+* Read the merged content from `result_buf.ptr` and `result_buf.size`.
+
+* Release buffers when finished.  A simple
+  `free(ancestor.ptr); free(ours.ptr); free(theirs.ptr);
+  free(result_buf.ptr);` will do.
 
 If the modifications do not merge cleanly, `ll_merge` will return a
 nonzero value and `result_buf` will generally include a description of
@@ -47,18 +90,6 @@ The `ancestor_label`, `our_label`, and `their_label` parameters are
 used to label the different sides of a conflict if the merge driver
 supports this.
 
-The `flag` parameter is a bitfield:
-
- - The `LL_OPT_VIRTUAL_ANCESTOR` bit indicates whether this is an
-   internal merge to consolidate ancestors for a recursive merge.
-
- - The `LL_OPT_FAVOR_MASK` bits allow local conflicts to be automatically
-   resolved in favor of one side or the other (as in 'git merge-file'
-   `--ours`/`--theirs`/`--union`).
-   They can be populated by `create_ll_flag`, whose argument can be
-   `XDL_MERGE_FAVOR_OURS`, `XDL_MERGE_FAVOR_THEIRS`, or
-   `XDL_MERGE_FAVOR_UNION`.
-
 Everything else
 ---------------
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 24b67d5..4d36b28 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -155,7 +155,7 @@ static int checkout_merged(int pos, struct checkout *state)
 	 * merge.renormalize set, too
 	 */
 	status = ll_merge(&result_buf, path, &ancestor, "base",
-			  &ours, "ours", &theirs, "theirs", 0);
+			  &ours, "ours", &theirs, "theirs", NULL);
 	free(ancestor.ptr);
 	free(ours.ptr);
 	free(theirs.ptr);
diff --git a/ll-merge.c b/ll-merge.c
index 6bb3095..9bd3732 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -18,7 +18,7 @@ typedef int (*ll_merge_fn)(const struct ll_merge_driver *,
 			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
-			   int flag,
+			   const struct ll_merge_options *opts,
 			   int marker_size);
 
 struct ll_merge_driver {
@@ -39,14 +39,18 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
 			   mmfile_t *src2, const char *name2,
-			   int flag, int marker_size)
+			   const struct ll_merge_options *opts,
+			   int marker_size)
 {
+	mmfile_t *stolen;
+	assert(opts);
+
 	/*
 	 * The tentative merge result is "ours" for the final round,
 	 * or common ancestor for an internal merge.  Still return
 	 * "conflicted merge" status.
 	 */
-	mmfile_t *stolen = (flag & LL_OPT_VIRTUAL_ANCESTOR) ? orig : src1;
+	stolen = opts->virtual_ancestor ? orig : src1;
 
 	result->ptr = stolen->ptr;
 	result->size = stolen->size;
@@ -60,9 +64,11 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
-			int flag, int marker_size)
+			const struct ll_merge_options *opts,
+			int marker_size)
 {
 	xmparam_t xmp;
+	assert(opts);
 
 	if (buffer_is_binary(orig->ptr, orig->size) ||
 	    buffer_is_binary(src1->ptr, src1->size) ||
@@ -74,12 +80,12 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 				       orig, orig_name,
 				       src1, name1,
 				       src2, name2,
-				       flag, marker_size);
+				       opts, marker_size);
 	}
 
 	memset(&xmp, 0, sizeof(xmp));
 	xmp.level = XDL_MERGE_ZEALOUS;
-	xmp.favor = ll_opt_favor(flag);
+	xmp.favor = opts->variant;
 	if (git_xmerge_style >= 0)
 		xmp.style = git_xmerge_style;
 	if (marker_size > 0)
@@ -96,14 +102,17 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,
 			  mmfile_t *orig, const char *orig_name,
 			  mmfile_t *src1, const char *name1,
 			  mmfile_t *src2, const char *name2,
-			  int flag, int marker_size)
+			  const struct ll_merge_options *opts,
+			  int marker_size)
 {
 	/* Use union favor */
-	flag &= ~LL_OPT_FAVOR_MASK;
-	flag |= create_ll_flag(XDL_MERGE_FAVOR_UNION);
+	struct ll_merge_options o;
+	assert(opts);
+	o = *opts;
+	o.variant = XDL_MERGE_FAVOR_UNION;
 	return ll_xdl_merge(drv_unused, result, path_unused,
 			    orig, NULL, src1, NULL, src2, NULL,
-			    flag, marker_size);
+			    &o, marker_size);
 	return 0;
 }
 
@@ -136,7 +145,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
 			mmfile_t *src2, const char *name2,
-			int flag, int marker_size)
+			const struct ll_merge_options *opts,
+			int marker_size)
 {
 	char temp[4][50];
 	struct strbuf cmd = STRBUF_INIT;
@@ -144,6 +154,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 	const char *args[] = { NULL, NULL };
 	int status, fd, i;
 	struct stat st;
+	assert(opts);
 
 	dict[0].placeholder = "O"; dict[0].value = temp[0];
 	dict[1].placeholder = "A"; dict[1].value = temp[1];
@@ -337,15 +348,21 @@ int ll_merge(mmbuffer_t *result_buf,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
 	     mmfile_t *theirs, const char *their_label,
-	     int flag)
+	     const struct ll_merge_options *opts)
 {
 	static struct git_attr_check check[2];
 	const char *ll_driver_name = NULL;
 	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	const struct ll_merge_driver *driver;
-	int virtual_ancestor = flag & LL_OPT_VIRTUAL_ANCESTOR;
 
-	if (flag & LL_OPT_RENORMALIZE) {
+	if (!opts) {
+		struct ll_merge_options default_opts = {0};
+		return ll_merge(result_buf, path, ancestor, ancestor_label,
+				ours, our_label, theirs, their_label,
+				&default_opts);
+	}
+
+	if (opts->renormalize) {
 		normalize_file(ancestor, path);
 		normalize_file(ours, path);
 		normalize_file(theirs, path);
@@ -359,11 +376,11 @@ int ll_merge(mmbuffer_t *result_buf,
 		}
 	}
 	driver = find_ll_merge_driver(ll_driver_name);
-	if (virtual_ancestor && driver->recursive)
+	if (opts->virtual_ancestor && driver->recursive)
 		driver = find_ll_merge_driver(driver->recursive);
 	return driver->fn(driver, result_buf, path, ancestor, ancestor_label,
 			  ours, our_label, theirs, their_label,
-			  flag, marker_size);
+			  opts, marker_size);
 }
 
 int ll_merge_marker_size(const char *path)
diff --git a/ll-merge.h b/ll-merge.h
index ff7ca87..4b707f0 100644
--- a/ll-merge.h
+++ b/ll-merge.h
@@ -5,27 +5,18 @@
 #ifndef LL_MERGE_H
 #define LL_MERGE_H
 
-#define LL_OPT_VIRTUAL_ANCESTOR	(1 << 0)
-#define LL_OPT_FAVOR_MASK	((1 << 1) | (1 << 2))
-#define LL_OPT_FAVOR_SHIFT 1
-#define LL_OPT_RENORMALIZE	(1 << 3)
-
-static inline int ll_opt_favor(int flag)
-{
-	return (flag & LL_OPT_FAVOR_MASK) >> LL_OPT_FAVOR_SHIFT;
-}
-
-static inline int create_ll_flag(int favor)
-{
-	return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK);
-}
+struct ll_merge_options {
+	unsigned virtual_ancestor : 1;
+	unsigned variant : 2;	/* favor ours, favor theirs, or union merge */
+	unsigned renormalize : 1;
+};
 
 int ll_merge(mmbuffer_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
 	     mmfile_t *theirs, const char *their_label,
-	     int flag);
+	     const struct ll_merge_options *opts);
 
 int ll_merge_marker_size(const char *path);
 
diff --git a/merge-file.c b/merge-file.c
index db4d0d5..f7f4533 100644
--- a/merge-file.c
+++ b/merge-file.c
@@ -37,7 +37,7 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our
 	 * common ancestor.
 	 */
 	merge_status = ll_merge(&res, path, base, NULL,
-				our, ".our", their, ".their", 0);
+				our, ".our", their, ".their", NULL);
 	if (merge_status < 0)
 		return NULL;
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 44576b7..0e9a29d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -608,22 +608,25 @@ static int merge_3way(struct merge_options *o,
 		      const char *branch2)
 {
 	mmfile_t orig, src1, src2;
+	struct ll_merge_options ll_opts = {0};
 	char *base_name, *name1, *name2;
 	int merge_status;
-	int favor;
 
-	if (o->call_depth)
-		favor = 0;
-	else {
+	ll_opts.renormalize = o->renormalize;
+
+	if (o->call_depth) {
+		ll_opts.virtual_ancestor = 1;
+		ll_opts.variant = 0;
+	} else {
 		switch (o->recursive_variant) {
 		case MERGE_RECURSIVE_OURS:
-			favor = XDL_MERGE_FAVOR_OURS;
+			ll_opts.variant = XDL_MERGE_FAVOR_OURS;
 			break;
 		case MERGE_RECURSIVE_THEIRS:
-			favor = XDL_MERGE_FAVOR_THEIRS;
+			ll_opts.variant = XDL_MERGE_FAVOR_THEIRS;
 			break;
 		default:
-			favor = 0;
+			ll_opts.variant = 0;
 			break;
 		}
 	}
@@ -646,10 +649,7 @@ static int merge_3way(struct merge_options *o,
 	read_mmblob(&src2, b->sha1);
 
 	merge_status = ll_merge(result_buf, a->path, &orig, base_name,
-				&src1, name1, &src2, name2,
-				((o->call_depth ? LL_OPT_VIRTUAL_ANCESTOR : 0) |
-				 (o->renormalize ? LL_OPT_RENORMALIZE : 0) |
-				 create_ll_flag(favor)));
+				&src1, name1, &src2, name2, &ll_opts);
 
 	free(name1);
 	free(name2);
diff --git a/rerere.c b/rerere.c
index e40af0d..b180218 100644
--- a/rerere.c
+++ b/rerere.c
@@ -325,7 +325,7 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 	 */
 	ll_merge(&result, path, &mmfile[0], NULL,
 		 &mmfile[1], "ours",
-		 &mmfile[2], "theirs", 0);
+		 &mmfile[2], "theirs", NULL);
 	for (i = 0; i < 3; i++)
 		free(mmfile[i].ptr);
 
-- 
1.7.2.2

Re: [PATCH 2/4] ll-merge: replace flag argument with options struct

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Keeping track of the flag bits is proving more trouble than it's
> worth.  Instead, use a pointer to an options struct like most similar
> APIs do.
>
> Callers with no special requests can pass NULL to request the default
> options.
>
> Cc: Bert Wesarg <bert.wesarg@googlemail.com>
> Cc: Avery Pennarun <apenwarr@gmail.com>
> Helped-by: Justin Frankel <justin@cockos.com>
> Helped-by: Bert Wesarg <bert.wesarg@googlemail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> This time, with updated documentation.

Thanks.

> diff --git a/ll-merge.c b/ll-merge.c
> index 6bb3095..9bd3732 100644
> --- a/ll-merge.c
> +++ b/ll-merge.c
> ...
> @@ -96,14 +102,17 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,
>  			  mmfile_t *orig, const char *orig_name,
>  			  mmfile_t *src1, const char *name1,
>  			  mmfile_t *src2, const char *name2,
> -			  int flag, int marker_size)
> +			  const struct ll_merge_options *opts,
> +			  int marker_size)
>  {
>  	/* Use union favor */
> -	flag &= ~LL_OPT_FAVOR_MASK;
> -	flag |= create_ll_flag(XDL_MERGE_FAVOR_UNION);
> +	struct ll_merge_options o;
> +	assert(opts);
> +	o = *opts;
> +	o.variant = XDL_MERGE_FAVOR_UNION;
>  	return ll_xdl_merge(drv_unused, result, path_unused,
>  			    orig, NULL, src1, NULL, src2, NULL,
> -			    flag, marker_size);
> +			    &o, marker_size);
>  	return 0;

Hmph, two returns...

> @@ -337,15 +348,21 @@ int ll_merge(mmbuffer_t *result_buf,
>  	     mmfile_t *ancestor, const char *ancestor_label,
>  	     mmfile_t *ours, const char *our_label,
>  	     mmfile_t *theirs, const char *their_label,
> -	     int flag)
> +	     const struct ll_merge_options *opts)
>  {
>  	static struct git_attr_check check[2];
>  	const char *ll_driver_name = NULL;
>  	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>  	const struct ll_merge_driver *driver;
> -	int virtual_ancestor = flag & LL_OPT_VIRTUAL_ANCESTOR;
>  
> +	if (!opts) {
> +		struct ll_merge_options default_opts = {0};
> +		return ll_merge(result_buf, path, ancestor, ancestor_label,
> +				ours, our_label, theirs, their_label,
> +				&default_opts);

Fun---expecting tail recursion elimination ;-)?

> +	}
> +
> -	if (flag & LL_OPT_RENORMALIZE) {
> +	if (opts->renormalize) {
>  		normalize_file(ancestor, path);
>  		normalize_file(ours, path);
>  		normalize_file(theirs, path);
> ...
>  		}
>  	}
>  	driver = find_ll_merge_driver(ll_driver_name);

A tangent, as this comment is not about the "richer ll-merge" series.

The above strikes me that the low level merge driver *ought* to have a say
in the use of renormalize.  For example, ll_merge_binary() may probably
not want to have its input renormalized, no?

[PATCH v1.7.4-rc2] ll-merge: simplify opts == NULL case

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> +	if (!opts) {
>> +		struct ll_merge_options default_opts = {0};
>> +		return ll_merge(result_buf, path, ancestor, ancestor_label,
>> +				ours, our_label, theirs, their_label,
>> +				&default_opts);
>
> Fun---expecting tail recursion elimination ;-)?

Fun but not warranted.  Let's simplify.

-- 8< --
Subject: ll-merge: simplify opts == NULL case

As long as sizeof(struct ll_merge_options) is small, there is not
much reason not to keep a copy of the default merge options in the BSS
section.  In return, we get clearer code and one less stack frame in
the opts == NULL case.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 ll-merge.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/ll-merge.c b/ll-merge.c
index 007dd3e..6ce512e 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -351,16 +351,13 @@ int ll_merge(mmbuffer_t *result_buf,
 	     const struct ll_merge_options *opts)
 {
 	static struct git_attr_check check[2];
+	static const struct ll_merge_options default_opts;
 	const char *ll_driver_name = NULL;
 	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	const struct ll_merge_driver *driver;
 
-	if (!opts) {
-		struct ll_merge_options default_opts = {0};
-		return ll_merge(result_buf, path, ancestor, ancestor_label,
-				ours, our_label, theirs, their_label,
-				&default_opts);
-	}
+	if (!opts)
+		opts = &default_opts;
 
 	if (opts->renormalize) {
 		normalize_file(ancestor, path);
-- 
1.7.4.rc2

[PATCH 3/4] merge-recursive --patience

[Jonathan Nieder]

From: Justin Frankel <justin@cockos.com>

Teach the merge-recursive strategy a --patience option to use the
"patience diff" algorithm, which tends to improve results when
cherry-picking a patch that reorders functions at the same time as
refactoring them.

To support this, struct merge_options and ll_merge_options gain an
xdl_opts member, so programs can use arbitrary xdiff flags (think
"XDF_IGNORE_WHITESPACE") in a git-aware merge.

git merge and git rebase can be passed the -Xpatience option to
use this.

[jn: split from --ignore-space patch; with documentation]

Signed-off-by: Justin Frankel <justin@cockos.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/merge-strategies.txt |    7 +++++++
 builtin/merge-recursive.c          |    1 +
 ll-merge.c                         |    1 +
 ll-merge.h                         |    1 +
 merge-recursive.c                  |    3 +++
 merge-recursive.h                  |    1 +
 6 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 049313d..a5ae14f 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -40,6 +40,13 @@ the other tree did, declaring 'our' history contains all that happened in it.
 theirs;;
 	This is opposite of 'ours'.
 
+patience;;
+	With this option, 'merge-recursive' spends a little extra time
+	to avoid mismerges that sometimes occur due to unimportant
+	matching lines (e.g., braces from distinct functions).  Use
+	this when the branches to be merged have diverged wildly.
+	See also linkgit:git-diff[1] `--patience`.
+
 renormalize;;
 	This runs a virtual check-out and check-in of all three stages
 	of a file when resolving a three-way merge.  This option is
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 5a52f3d..70e1d25 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -2,6 +2,7 @@
 #include "commit.h"
 #include "tag.h"
 #include "merge-recursive.h"
+#include "xdiff-interface.h"
 
 static const char *better_branch_name(const char *branch)
 {
diff --git a/ll-merge.c b/ll-merge.c
index 9bd3732..dea6f2f 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -86,6 +86,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	memset(&xmp, 0, sizeof(xmp));
 	xmp.level = XDL_MERGE_ZEALOUS;
 	xmp.favor = opts->variant;
+	xmp.xpp.flags = opts->xdl_opts;
 	if (git_xmerge_style >= 0)
 		xmp.style = git_xmerge_style;
 	if (marker_size > 0)
diff --git a/ll-merge.h b/ll-merge.h
index 4b707f0..244a31f 100644
--- a/ll-merge.h
+++ b/ll-merge.h
@@ -9,6 +9,7 @@ struct ll_merge_options {
 	unsigned virtual_ancestor : 1;
 	unsigned variant : 2;	/* favor ours, favor theirs, or union merge */
 	unsigned renormalize : 1;
+	long xdl_opts;
 };
 
 int ll_merge(mmbuffer_t *result_buf,
diff --git a/merge-recursive.c b/merge-recursive.c
index 0e9a29d..3e67f81 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -613,6 +613,7 @@ static int merge_3way(struct merge_options *o,
 	int merge_status;
 
 	ll_opts.renormalize = o->renormalize;
+	ll_opts.xdl_opts = o->xdl_opts;
 
 	if (o->call_depth) {
 		ll_opts.virtual_ancestor = 1;
@@ -1512,6 +1513,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		o->subtree_shift = "";
 	else if (!prefixcmp(s, "subtree="))
 		o->subtree_shift = s + strlen("subtree=");
+	else if (!strcmp(s, "patience"))
+		o->xdl_opts |= XDF_PATIENCE_DIFF;
 	else if (!strcmp(s, "renormalize"))
 		o->renormalize = 1;
 	else if (!strcmp(s, "no-renormalize"))
diff --git a/merge-recursive.h b/merge-recursive.h
index 37ff99a..d21b446 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -15,6 +15,7 @@ struct merge_options {
 	const char *subtree_shift;
 	unsigned buffer_output : 1;
 	unsigned renormalize : 1;
+	long xdl_opts;
 	int verbosity;
 	int diff_rename_limit;
 	int merge_rename_limit;
-- 
1.7.2.2

[PATCH 4/4] merge-recursive: options to ignore whitespace changes

[Jonathan Nieder]

From: Justin Frankel <justin@cockos.com>

Add support for merging with ignoring line endings (specifically
--ignore-space-at-eol) when using recursive merging.  This is
as a strategy-option, so that you can do:

	git merge --strategy-option=ignore-space-at-eol <branch>

and

	git rebase --strategy-option=ignore-space-at-eol <branch>

This can be useful for coping with line-ending damage (Xcode 3.1 has a
nasty habit of converting all CRLFs to LFs, and VC6 tends to just use
CRLFs for inserted lines).

The only option I need is ignore-space-at-eol, but while at it,
include the other xdiff whitespace options (ignore-space-change,
ignore-all-space), too.

[jn: with documentation]

Signed-off-by: Justin Frankel <justin@cockos.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's the end of the series.  Thoughts (and tests) welcome, as
always.

 Documentation/merge-strategies.txt |   15 +++++++++++++++
 merge-recursive.c                  |    8 ++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index a5ae14f..91faba5 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -47,6 +47,21 @@ patience;;
 	this when the branches to be merged have diverged wildly.
 	See also linkgit:git-diff[1] `--patience`.
 
+ignore-space-change;;
+ignore-all-space;;
+ignore-space-at-eol;;
+	Treats lines with the indicated type of whitespace change as
+	unchanged for the sake of a three-way merge.  Whitespace
+	changes mixed with other changes to a line are not ignored.
+	See also linkgit:git-diff[1] `-b`, `-w`, and
+	`--ignore-space-at-eol`.
++
+* If 'their' version only introduces whitespace changes to a line,
+  'our' version is used;
+* If 'our' version introduces whitespace changes but 'their'
+  version includes a substantial change, 'their' version is used;
+* Otherwise, the merge proceeds in the usual way.
+
 renormalize;;
 	This runs a virtual check-out and check-in of all three stages
 	of a file when resolving a three-way merge.  This option is
diff --git a/merge-recursive.c b/merge-recursive.c
index 3e67f81..944ca19 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1515,6 +1515,14 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		o->subtree_shift = s + strlen("subtree=");
 	else if (!strcmp(s, "patience"))
 		o->xdl_opts |= XDF_PATIENCE_DIFF;
+	else if (!strcmp(s, "ignore-space-change"))
+		o->xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
+	else if (!strcmp(s, "ignore-all-space"))
+		o->xdl_opts |= XDF_IGNORE_WHITESPACE;
+	else if (!strcmp(s, "ignore-space-at-eol"))
+		o->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
+	else if (!strcmp(s, "ignore-space-at-eol"))
+		o->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
 	else if (!strcmp(s, "renormalize"))
 		o->renormalize = 1;
 	else if (!strcmp(s, "no-renormalize"))
-- 
1.7.2.2

Re: [PATCH 4/4] merge-recursive: options to ignore whitespace changes

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> From: Justin Frankel <justin@cockos.com>
>
> Add support for merging with ignoring line endings (specifically
> --ignore-space-at-eol) when using recursive merging.  This is
> as a strategy-option, so that you can do:

"This is as a strategy-option"?  s/is/& done/ perhaps?

Otherwise looks good to me.

Re: [PATCH 4/4] merge-recursive: options to ignore whitespace changes

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> From: Justin Frankel <justin@cockos.com>
>>
>> Add support for merging with ignoring line endings (specifically
>> --ignore-space-at-eol) when using recursive merging.  This is
>> as a strategy-option, so that you can do:
>
> "This is as a strategy-option"?  s/is/& done/ perhaps?

Sure, that makes it clearer.

> Otherwise looks good to me.

Hmph, one embarrasingly sloppy bit on my part:

+       else if (!strcmp(s, "ignore-space-at-eol"))
+               o->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
+       else if (!strcmp(s, "ignore-space-at-eol"))
+               o->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;

Here's a fixup, plus a quick test while at it.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 merge-recursive.c                  |    2 -
 t/t3032-merge-recursive-options.sh |  186 ++++++++++++++++++++++++++++++++++++
 2 files changed, 186 insertions(+), 2 deletions(-)
 create mode 100755 t/t3032-merge-recursive-options.sh

diff --git a/merge-recursive.c b/merge-recursive.c
index 944ca19..9b9f97e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1521,8 +1521,6 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		o->xdl_opts |= XDF_IGNORE_WHITESPACE;
 	else if (!strcmp(s, "ignore-space-at-eol"))
 		o->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
-	else if (!strcmp(s, "ignore-space-at-eol"))
-		o->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
 	else if (!strcmp(s, "renormalize"))
 		o->renormalize = 1;
 	else if (!strcmp(s, "no-renormalize"))
diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
new file mode 100755
index 0000000..2293797
--- /dev/null
+++ b/t/t3032-merge-recursive-options.sh
@@ -0,0 +1,186 @@
+#!/bin/sh
+
+test_description='merge-recursive options
+
+* [master] Clarify
+ ! [remote] Remove cruft
+--
+ + [remote] Remove cruft
+*  [master] Clarify
+*+ [remote^] Initial revision
+*   ok 1: setup
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	conflict_hunks () {
+		sed -n -e "
+			/^<<<</ b inconflict
+			b
+			: inconflict
+			p
+			/^>>>>/ b
+			n
+			b inconflict
+		" "$@"
+	} &&
+
+	cat <<-\EOF >text.txt &&
+	    Hope, he says, cherishes the soul of him who lives in
+	    justice and holiness and is the nurse of his age and the
+	    companion of his journey;--hope which is mightiest to sway
+	    the restless soul of man.
+
+	How admirable are his words!  And the great blessing of riches, I do
+	not say to every man, but to a good man, is, that he has had no
+	occasion to deceive or to defraud others, either intentionally or
+	unintentionally; and when he departs to the world below he is not in
+	any apprehension about offerings due to the gods or debts which he owes
+	to men.  Now to this peace of mind the possession of wealth greatly
+	contributes; and therefore I say, that, setting one thing against
+	another, of the many advantages which wealth has to give, to a man of
+	sense this is in my opinion the greatest.
+
+	Well said, Cephalus, I replied; but as concerning justice, what is
+	it?--to speak the truth and to pay your debts--no more than this?  And
+	even to this are there not exceptions?  Suppose that a friend when in
+	his right mind has deposited arms with me and he asks for them when he
+	is not in his right mind, ought I to give them back to him?  No one
+	would say that I ought or that I should be right in doing so, any more
+	than they would say that I ought always to speak the truth to one who
+	is in his condition.
+
+	You are quite right, he replied.
+
+	But then, I said, speaking the truth and paying your debts is not a
+	correct definition of justice.
+
+	CEPHALUS - SOCRATES - POLEMARCHUS
+
+	Quite correct, Socrates, if Simonides is to be believed, said
+	Polemarchus interposing.
+
+	I fear, said Cephalus, that I must go now, for I have to look after the
+	sacrifices, and I hand over the argument to Polemarchus and the company.
+	EOF
+	git add text.txt &&
+	test_tick &&
+	git commit -m "Initial revision" &&
+
+	git checkout -b remote &&
+	sed -e "
+			s/\.  /\. /g
+			s/[?]  /? /g
+			s/    /	/g
+			s/--/---/g
+			s/but as concerning/but as con cerning/
+			/CEPHALUS - SOCRATES - POLEMARCHUS/ d
+		" text.txt >text.txt+ &&
+	mv text.txt+ text.txt &&
+	git commit -a -m "Remove cruft" &&
+
+	git checkout master &&
+	sed -e "
+			s/\(not in his right mind\),\(.*\)/\1;\2Q/
+			s/Quite correct\(.*\)/It is too correct\1Q/
+			s/unintentionally/un intentionally/
+			/un intentionally/ s/$/Q/
+			s/Polemarchus interposing./Polemarchus, interposing.Q/
+			/justice and holiness/ s/$/Q/
+			/pay your debts/ s/$/Q/
+		" text.txt | q_to_cr >text.txt+ &&
+	mv text.txt+ text.txt &&
+	git commit -a -m "Clarify" &&
+	git show-branch --all
+'
+
+test_expect_success 'naive merge fails' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive HEAD^ -- HEAD remote &&
+	test_must_fail git update-index --refresh &&
+	grep "<<<<<<" text.txt
+'
+
+test_expect_success '--ignore-space-change makes merge succeed' '
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --ignore-space-change HEAD^ -- HEAD remote
+'
+
+test_expect_success '--ignore-space-change: our w/s-only change wins' '
+	q_to_cr <<-\EOF >expected &&
+	    justice and holiness and is the nurse of his age and theQ
+	EOF
+
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --ignore-space-change HEAD^ -- HEAD remote &&
+	grep "justice and holiness" text.txt >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--ignore-space-change: their real change wins over w/s' '
+	cat <<-\EOF >expected &&
+	it?---to speak the truth and to pay your debts---no more than this? And
+	EOF
+
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --ignore-space-change HEAD^ -- HEAD remote &&
+	grep "pay your debts" text.txt >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--ignore-space-change: does not ignore new spaces' '
+	cat <<-\EOF >expected1 &&
+	Well said, Cephalus, I replied; but as con cerning justice, what is
+	EOF
+	q_to_cr <<-\EOF >expected2 &&
+	un intentionally; and when he departs to the world below he is not inQ
+	EOF
+
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --ignore-space-change HEAD^ -- HEAD remote &&
+	grep "Well said" text.txt >actual1 &&
+	grep "when he departs" text.txt >actual2 &&
+	test_cmp expected1 actual1 &&
+	test_cmp expected2 actual2
+'
+
+test_expect_success '--ignore-all-space drops their new spaces' '
+	cat <<-\EOF >expected &&
+	Well said, Cephalus, I replied; but as concerning justice, what is
+	EOF
+
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --ignore-all-space HEAD^ -- HEAD remote &&
+	grep "Well said" text.txt >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--ignore-all-space keeps our new spaces' '
+	q_to_cr <<-\EOF >expected &&
+	un intentionally; and when he departs to the world below he is not inQ
+	EOF
+
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --ignore-all-space HEAD^ -- HEAD remote &&
+	grep "when he departs" text.txt >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--ignore-space-at-eol' '
+	q_to_cr <<-\EOF >expected &&
+	<<<<<<< HEAD
+	is not in his right mind; ought I to give them back to him?  No oneQ
+	=======
+	is not in his right mind, ought I to give them back to him? No one
+	>>>>>>> remote
+	EOF
+
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --ignore-space-at-eol \
+						 HEAD^ -- HEAD remote &&
+	conflict_hunks text.txt >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
1.7.2.2