Re: [PATCH v2 3/8] ll-merge: make callers responsible for showing warnings

On Tue, Dec 28, 2021 at 2:56 AM Johannes Altmanninger [off-list ref] wrote:

On Sat, Dec 25, 2021 at 07:59:14AM +0000, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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.

This commit continues printing the message as-is; future changes will
start handling the new commit differently in the merge-ort codepath.

"the new commit" looks like a typo, do you mean "binary conflicts"?

Good catch, yeah should be "the binary conflicts message"

quoted

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.

Heh, this is a clever way to work around C's weak typing.

The language server I'm using (clangd) supports the Call Hierarchy feature,
which is intended to list callers or callees of the function at the editor's
cursor. If I ask the server for callers of ll_merge I get this response
(on 510f9eba9 plus this series)

        ll-merge.h:98:1: ll_merge - list of callers
          builtin/checkout.c:242:12: checkout_merged
            builtin/checkout.c:279:17:  merge_status = ll_merge(&result_buf, path, &ancestor, "base",
          rerere.c:943:12: handle_cache
            rerere.c:984:2:     ll_merge(&result, path, &mmfile[0], NULL,
          notes-merge.c:342:12: ll_merge_in_worktree
            notes-merge.c:353:11:       status = ll_merge(&result_buf, oid_to_hex(&p->obj), &base, NULL,
          merge-recursive.c:1035:12: merge_3way
            merge-recursive.c:1090:17:  merge_status = ll_merge(result_buf, a->path, &orig, base,
          merge-ort.c:1763:12: merge_3way
            merge-ort.c:1816:17:        merge_status = ll_merge(result_buf, path, &orig, base,
          merge-blobs.c:32:14: three_way_filemerge
            merge-blobs.c:48:17:        merge_status = ll_merge(&res, path, base, NULL,
          apply.c:3491:12: three_way_merge
            apply.c:3511:11:    status = ll_merge(&result, path,
          rerere.c:608:12: try_merge
            rerere.c:623:9:             ret = ll_merge(result, path, &base, NULL, cur, "", &other, "",

So there are 8 callers in total; but only 7 print the warning (including the
one in merge-ort which will change in the next commit). I think you missed
the call at rerere.c:984 because we ignore its return value.

Doh, I missed one!  Though, as pointed out by Junio, rerere won't
operate on binary files and thus can't hit that codepath.  Still, I
should either have it in both rerere codepaths or neither.

quoted

@@ -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 > 1 ) ? LL_MERGE_CONFLICT : status;

" (status > 1 )" has an extra space

I'm not sure it's wise to handle status=1 and status=2 in two different code paths.
Both mean the same (the only difference is the number of conflicts).
status=1 coincides with LL_MERGE_CONFLICT but that's purely coincidental

        ret = (status > 0) ? LL_MERGE_CONFLICT : status;

Um, whoops.  Yeah, this should be > 0, not > 1.  (As per
xdl_do_merge() comment, status >= 0 means status returns the number of
conflicts)  No clue how I messed that up so badly; kind of
embarrassing, honestly.

Thanks for the careful reading.

quoted

@@ -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 > 1) ? LL_MERGE_CONFLICT : status;

same here, I'd test for "status > 0" because that's the convention for
external programs

Yep.

...

quoted

diff --git a/rerere.c b/rerere.c
index d83d58df4fb..b1f8961ed9e 100644
--- a/rerere.c
+++ b/rerere.c

@@ -609,19 +609,23 @@ 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);
+             if (ret == LL_MERGE_BINARY_CONFLICT)
+                     warning("Cannot merge binary files: %s (%s vs. %s)",
+                             path, "", "");

With the next patch, 7/8 callers of ll_merge (almost) immediately print
that warning.  Looks fine as is, but does it make sense to introduce a helper
function for the common case, or add a flag to ll_merge_options?

I started by adding a flag, and Peff suggested not doing so (because
the printing doesn't belong in a "low-level" merge, as ll_merge stands
for[1]), but instead making the callers responsible.  We could add a
helper function, outside of ll-merge.[ch], but I'm not sure where to
put it or what to call it and I'm leaning towards just leaving things
as-is (well, other than fixing up the important issues you brought up
before this).

[1] https://lore.kernel.org/git/YVOZRhWttzF18Xql@coredump.intra.peff.net/ (local)