Re: [PATCH 3/7] ll-merge: add API for capturing warnings in a strbuf instead of stderr
From: Jeff King <hidden>
Date: 2021-09-28 22:38:02
On Tue, Aug 31, 2021 at 02:26:36AM +0000, Elijah Newren via GitGitGadget wrote:
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 hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.
-Peff