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@xxxxxxxxxxxxxx>
Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html