On Tue, Sep 28, 2021 at 3:37 PM Jeff King <peff@xxxxxxxx> wrote:
>
> 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.
Ah, thanks for catching that. I can fix it up.
> > @@ -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...
