Re: [PATCH 3/7] ll-merge: add API for capturing warnings in a strbuf instead of stderr

On Tue, Sep 28, 2021 at 3:37 PM Jeff King [off-list ref] wrote:

On Tue, Aug 31, 2021 at 02:26:36AM +0000, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

Whenever ll-merge encounters a binary file, it prints a warning to
stderr.  However, for the --remerge-diff option we want to add, we need
to capture all conflict messages and show them in a diff instead of
dumping them straight to stdout or stderr.  Add some new API that will
allow us to capture this output and display it in our preferred method.

This is a reasonable strategy for error-handling in general (though some
more thoughts below).

quoted

@@ -71,8 +75,11 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
      } else {
              switch (opts->variant) {
              default:
-                     warning("Cannot merge binary files: %s (%s vs. %s)",
-                             path, name1, name2);
+                     if (warnings) {
+                             strbuf_addstr(warnings, "Warning: ");
+                             strbuf_addf(warnings, msg, path, name1, name2);
+                     } else
+                             warning(msg, path, name1, name2);

The usual warn_builtin() has a lowercase "warning: " prefix, but your
strbuf variant uses uppercase.

Ah, thanks for catching that.  I can fix it up.

quoted

@@ -98,6 +105,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,

 static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
                      mmbuffer_t *result,
+                     struct strbuf *warnings,
                      const char *path,
                      mmfile_t *orig, const char *orig_name,
                      mmfile_t *src1, const char *name1,

There's a lot of plumbing this variable through. This is probably too
gross, but another option would be to call set_warn_routine() to
override it temporarily. It's gross because it affects everything, not
just this call stack (and I also think we'd need to beef up the warn
routine code to handle some of the rough edges).

I do wonder if the ll_merge() code should avoid calling warning() in the
first place. It is after all, meant to be "low-level". We already return
an error code from the function. I wonder if returning a more detailed
code instead, like:

  enum LL_MERGE_RESULT {
        LL_MERGE_OK = 0,
        LL_MERGE_CONFLICT,
        LL_MERGE_BINARY_CONFLICT,
  };

would let the caller do the sensible thing.

Ooh, I like this idea.  I'll have to change all the callers, but
there's only about half a dozen or so...