Re: [PATCH v2 3/8] ll-merge: make callers responsible for showing warnings

> On Tue, Dec 28, 2021 at 2:56 AM Johannes Altmanninger <aclopte@gmail.com> wrote:
> >
> > On Sat, Dec 25, 2021 at 07:59:14AM +0000, Elijah Newren via GitGitGadget wrote:
> >
> > So there are 8 callers in total; but only 7 print the warning (including the
> > one in merge-ort which will change in the next commit). I think you missed
> > the call at rerere.c:984 because we ignore its return value.
> 
> Doh, I missed one!  Though, as pointed out by Junio, rerere won't
> operate on binary files and thus can't hit that codepath.  Still, I
> should either have it in both rerere codepaths or neither.

> > > +             if (ret == LL_MERGE_BINARY_CONFLICT)
> > > +                     warning("Cannot merge binary files: %s (%s vs. %s)",
> > > +                             path, "", "");
> >
> > With the next patch, 7/8 callers of ll_merge (almost) immediately print
> > that warning.  Looks fine as is, but does it make sense to introduce a helper
> > function for the common case, or add a flag to ll_merge_options?
> 
> I started by adding a flag, and Peff suggested not doing so (because
> the printing doesn't belong in a "low-level" merge, as ll_merge stands
> for[1]), but instead making the callers responsible.  We could add a
> helper function, outside of ll-merge.[ch], but I'm not sure where to
> put it or what to call it and I'm leaning towards just leaving things
> as-is (well, other than fixing up the important issues you brought up
> before this).