ll-merge: make callers responsible for showing warnings

Since some callers may want to send warning messages to somewhere other
than stdout/stderr, stop printing "warning: Cannot merge binary files"
from ll-merge and instead modify the return status of ll_merge() to
indicate when a merge of binary files has occurred.  Message printing
probably does not belong in a "low-level merge" anyway.

This commit continues printing the message as-is, just from the callers
instead of within ll_merge().  Future changes will start handling the
message differently in the merge-ort codepath.

There was one special case here: the callers in rerere.c do NOT check
for and print such a message; since those code paths explicitly skip
over binary files, there is no reason to check for a return status of
LL_MERGE_BINARY_CONFLICT or print the related message.

Note that my methodology included first modifying ll_merge() to return
a struct, so that the compiler would catch all the callers for me and
ensure I had modified all of them.  After modifying all of them, I then
changed the struct to an enum.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/apply.c b/apply.c
index 43a0aebf4eec85fa6b0a69f655c0224aa14f29e5..8079395755f76c0fb4140f03767f82d65735b28a 100644 (file)
--- a/apply.c
+++ b/apply.c
@@ -3492,7 +3492,7 @@ static int three_way_merge(struct apply_state *state,
 {
        mmfile_t base_file, our_file, their_file;
        mmbuffer_t result = { NULL };
-       int status;
+       enum ll_merge_result status;
 
        /* resolve trivial cases first */
        if (oideq(base, ours))
@@ -3509,6 +3509,9 @@ static int three_way_merge(struct apply_state *state,
                          &their_file, "theirs",
                          state->repo->index,
                          NULL);
+       if (status == LL_MERGE_BINARY_CONFLICT)
+               warning("Cannot merge binary files: %s (%s vs. %s)",
+                       path, "ours", "theirs");
        free(base_file.ptr);
        free(our_file.ptr);
        free(their_file.ptr);

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 72beeb49aa90c290724cfcdfe905d67ce3224f89..53d7d7951a63a9e5c92e547d67e613ff1d4837db 100644 (file)
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -245,6 +245,7 @@ static int checkout_merged(int pos, const struct checkout *state,
        struct cache_entry *ce = active_cache[pos];
        const char *path = ce->name;
        mmfile_t ancestor, ours, theirs;
+       enum ll_merge_result merge_status;
        int status;
        struct object_id oid;
        mmbuffer_t result_buf;
@@ -275,13 +276,16 @@ static int checkout_merged(int pos, const struct checkout *state,
        memset(&ll_opts, 0, sizeof(ll_opts));
        git_config_get_bool("merge.renormalize", &renormalize);
        ll_opts.renormalize = renormalize;
-       status = ll_merge(&result_buf, path, &ancestor, "base",
-                         &ours, "ours", &theirs, "theirs",
-                         state->istate, &ll_opts);
+       merge_status = ll_merge(&result_buf, path, &ancestor, "base",
+                               &ours, "ours", &theirs, "theirs",
+                               state->istate, &ll_opts);
        free(ancestor.ptr);
        free(ours.ptr);
        free(theirs.ptr);
-       if (status < 0 || !result_buf.ptr) {
+       if (merge_status == LL_MERGE_BINARY_CONFLICT)
+               warning("Cannot merge binary files: %s (%s vs. %s)",
+                       path, "ours", "theirs");
+       if (merge_status < 0 || !result_buf.ptr) {
                free(result_buf.ptr);
                return error(_("path '%s': cannot merge"), path);
        }

diff --git a/ll-merge.c b/ll-merge.c
index 261657578c756c94f27ba03392ef82f4173d5460..a937cec59a6e8cd80f2272940ed0960a08d35125 100644 (file)
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -14,7 +14,7 @@
 
 struct ll_merge_driver;
 
-typedef int (*ll_merge_fn)(const struct ll_merge_driver *,
+typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,
                           mmbuffer_t *result,
                           const char *path,
                           mmfile_t *orig, const char *orig_name,
@@ -49,7 +49,7 @@ void reset_merge_attributes(void)
 /*
  * Built-in low-levels
  */
-static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
+static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv_unused,
                           mmbuffer_t *result,
                           const char *path,
                           mmfile_t *orig, const char *orig_name,
@@ -58,6 +58,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
                           const struct ll_merge_options *opts,
                           int marker_size)
 {
+       enum ll_merge_result ret;
        mmfile_t *stolen;
        assert(opts);
 
@@ -68,16 +69,19 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
         */
        if (opts->virtual_ancestor) {
                stolen = orig;
+               ret = LL_MERGE_OK;
        } else {
                switch (opts->variant) {
                default:
-                       warning("Cannot merge binary files: %s (%s vs. %s)",
-                               path, name1, name2);
-                       /* fallthru */
+                       ret = LL_MERGE_BINARY_CONFLICT;
+                       stolen = src1;
+                       break;
                case XDL_MERGE_FAVOR_OURS:
+                       ret = LL_MERGE_OK;
                        stolen = src1;
                        break;
                case XDL_MERGE_FAVOR_THEIRS:
+                       ret = LL_MERGE_OK;
                        stolen = src2;
                        break;
                }
@@ -87,16 +91,10 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
        result->size = stolen->size;
        stolen->ptr = NULL;
 
-       /*
-        * With -Xtheirs or -Xours, we have cleanly merged;
-        * otherwise we got a conflict.
-        */
-       return opts->variant == XDL_MERGE_FAVOR_OURS ||
-              opts->variant == XDL_MERGE_FAVOR_THEIRS ?
-              0 : 1;
+       return ret;
 }
 
-static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
+static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unused,
                        mmbuffer_t *result,
                        const char *path,
                        mmfile_t *orig, const char *orig_name,
@@ -105,7 +103,9 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
                        const struct ll_merge_options *opts,
                        int marker_size)
 {
+       enum ll_merge_result ret;
        xmparam_t xmp;
+       int status;
        assert(opts);
 
        if (orig->size > MAX_XDIFF_SIZE ||
@@ -133,10 +133,12 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
        xmp.ancestor = orig_name;
        xmp.file1 = name1;
        xmp.file2 = name2;
-       return xdl_merge(orig, src1, src2, &xmp, result);
+       status = xdl_merge(orig, src1, src2, &xmp, result);
+       ret = (status > 0) ? LL_MERGE_CONFLICT : status;
+       return ret;
 }
 
-static int ll_union_merge(const struct ll_merge_driver *drv_unused,
+static enum ll_merge_result ll_union_merge(const struct ll_merge_driver *drv_unused,
                          mmbuffer_t *result,
                          const char *path,
                          mmfile_t *orig, const char *orig_name,
@@ -178,7 +180,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 /*
  * User defined low-level merge driver support.
  */
-static int ll_ext_merge(const struct ll_merge_driver *fn,
+static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
                        mmbuffer_t *result,
                        const char *path,
                        mmfile_t *orig, const char *orig_name,
@@ -194,6 +196,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
        const char *args[] = { NULL, NULL };
        int status, fd, i;
        struct stat st;
+       enum ll_merge_result ret;
        assert(opts);
 
        sq_quote_buf(&path_sq, path);
@@ -236,7 +239,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
                unlink_or_warn(temp[i]);
        strbuf_release(&cmd);
        strbuf_release(&path_sq);
-       return status;
+       ret = (status > 0) ? LL_MERGE_CONFLICT : status;
+       return ret;
 }
 
 /*
@@ -362,7 +366,7 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i
        }
 }
 
-int ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
             const char *path,
             mmfile_t *ancestor, const char *ancestor_label,
             mmfile_t *ours, const char *our_label,

diff --git a/ll-merge.h b/ll-merge.h
index aceb1b24132306a272182556ee15003a1f0d4e08..e4a20e81a3aea90b64e65b1f33a38efa595d9a2a 100644 (file)
--- a/ll-merge.h
+++ b/ll-merge.h
@@ -82,13 +82,20 @@ struct ll_merge_options {
        long xdl_opts;
 };
 
+enum ll_merge_result {
+       LL_MERGE_ERROR = -1,
+       LL_MERGE_OK = 0,
+       LL_MERGE_CONFLICT,
+       LL_MERGE_BINARY_CONFLICT,
+};
+
 /**
  * Perform a three-way single-file merge in core.  This is a thin wrapper
  * around `xdl_merge` that takes the path and any merge backend specified in
  * `.gitattributes` or `.git/info/attributes` into account.
  * Returns 0 for a clean merge.
  */
-int ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
             const char *path,
             mmfile_t *ancestor, const char *ancestor_label,
             mmfile_t *ours, const char *our_label,

diff --git a/merge-blobs.c b/merge-blobs.c
index ee0a0e90c94682f4d990b298d27d734d01d90581..8138090f81cf726ee9834daa7edbfc5b542aec44 100644 (file)
--- a/merge-blobs.c
+++ b/merge-blobs.c
@@ -36,7 +36,7 @@ static void *three_way_filemerge(struct index_state *istate,
                                 mmfile_t *their,
                                 unsigned long *size)
 {
-       int merge_status;
+       enum ll_merge_result merge_status;
        mmbuffer_t res;
 
        /*
@@ -50,6 +50,9 @@ static void *three_way_filemerge(struct index_state *istate,
                                istate, NULL);
        if (merge_status < 0)
                return NULL;
+       if (merge_status == LL_MERGE_BINARY_CONFLICT)
+               warning("Cannot merge binary files: %s (%s vs. %s)",
+                       path, ".our", ".their");
 
        *size = res.size;
        return res.ptr;

diff --git a/merge-ort.c b/merge-ort.c
index 0342f104836b69a7889b3fa686c7c359c27e5dd6..c24da2ba3cb9d5b3f299feb9b39e11b0ba47a2bb 100644 (file)
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1743,7 +1743,7 @@ static int merge_3way(struct merge_options *opt,
        mmfile_t orig, src1, src2;
        struct ll_merge_options ll_opts = {0};
        char *base, *name1, *name2;
-       int merge_status;
+       enum ll_merge_result merge_status;
 
        if (!opt->priv->attr_index.initialized)
                initialize_attr_index(opt);
@@ -1787,6 +1787,9 @@ static int merge_3way(struct merge_options *opt,
        merge_status = ll_merge(result_buf, path, &orig, base,
                                &src1, name1, &src2, name2,
                                &opt->priv->attr_index, &ll_opts);
+       if (merge_status == LL_MERGE_BINARY_CONFLICT)
+               warning("Cannot merge binary files: %s (%s vs. %s)",
+                       path, name1, name2);
 
        free(base);
        free(name1);

diff --git a/merge-recursive.c b/merge-recursive.c
index d9457797dbb73bfed720ac9ca0b9bcf56575c62a..bc73c52dd847801cdbca80558ec4e0016d08813b 100644 (file)
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1044,7 +1044,7 @@ static int merge_3way(struct merge_options *opt,
        mmfile_t orig, src1, src2;
        struct ll_merge_options ll_opts = {0};
        char *base, *name1, *name2;
-       int merge_status;
+       enum ll_merge_result merge_status;
 
        ll_opts.renormalize = opt->renormalize;
        ll_opts.extra_marker_size = extra_marker_size;
@@ -1090,6 +1090,9 @@ static int merge_3way(struct merge_options *opt,
        merge_status = ll_merge(result_buf, a->path, &orig, base,
                                &src1, name1, &src2, name2,
                                opt->repo->index, &ll_opts);
+       if (merge_status == LL_MERGE_BINARY_CONFLICT)
+               warning("Cannot merge binary files: %s (%s vs. %s)",
+                       a->path, name1, name2);
 
        free(base);
        free(name1);

diff --git a/notes-merge.c b/notes-merge.c
index b4a3a903e86f3fdc38064e5f16c63974d2a191e4..01d596920eaf69f94325ea3cd7678963b67b7101 100644 (file)
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -344,7 +344,7 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,
 {
        mmbuffer_t result_buf;
        mmfile_t base, local, remote;
-       int status;
+       enum ll_merge_result status;
 
        read_mmblob(&base, &p->base);
        read_mmblob(&local, &p->local);
@@ -358,6 +358,9 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,
        free(local.ptr);
        free(remote.ptr);
 
+       if (status == LL_MERGE_BINARY_CONFLICT)
+               warning("Cannot merge binary files: %s (%s vs. %s)",
+                       oid_to_hex(&p->obj), o->local_ref, o->remote_ref);
        if ((status < 0) || !result_buf.ptr)
                die("Failed to execute internal merge");
 

diff --git a/rerere.c b/rerere.c
index d83d58df4fbc930b756395a8a99a77be4c1e598f..d26627c59329151ef196e26444a64cb44ad699d4 100644 (file)
--- a/rerere.c
+++ b/rerere.c
@@ -609,19 +609,20 @@ static int try_merge(struct index_state *istate,
                     const struct rerere_id *id, const char *path,
                     mmfile_t *cur, mmbuffer_t *result)
 {
-       int ret;
+       enum ll_merge_result ret;
        mmfile_t base = {NULL, 0}, other = {NULL, 0};
 
        if (read_mmfile(&base, rerere_path(id, "preimage")) ||
-           read_mmfile(&other, rerere_path(id, "postimage")))
-               ret = 1;
-       else
+           read_mmfile(&other, rerere_path(id, "postimage"))) {
+               ret = LL_MERGE_CONFLICT;
+       } else {
                /*
                 * A three-way merge. Note that this honors user-customizable
                 * low-level merge driver settings.
                 */
                ret = ll_merge(result, path, &base, NULL, cur, "", &other, "",
                               istate, NULL);
+       }
 
        free(base.ptr);
        free(other.ptr);