On Tue, Aug 31, 2021 at 02:26:36AM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> 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).
> @@ -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.
> @@ -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
