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

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