On Tue, Dec 28, 2021 at 11:37:01AM -0800, Elijah Newren wrote:
> On Tue, Dec 28, 2021 at 2:56 AM Johannes Altmanninger <aclopte@xxxxxxxxx> 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.
"neither" sounds good
> > > + 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).
Sure, leaving this sounds fine. If we can formulate good reasons against
the discarded approaches we should add them to the commit message. I guess
in this case the small number of call sites is a good indication that it's
probably not worth it.
