Re: [PATCH v2] git-merge: ignore space support
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:22
Subsystem:
the rest · Maintainer:
Linus Torvalds
Justin Frankel wrote:
This is for the next/ branch
Applies on top of jn/merge-renormalize. Thanks for the reroll.
quoted hunk ↗ jump to hunk
--- 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").
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
+++ 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 <redacted> Signed-off-by: Jonathan Nieder <redacted> --- 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