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

On Tue, Dec 28, 2021 at 11:37:01AM -0800, Elijah Newren wrote:

On Tue, Dec 28, 2021 at 2:56 AM Johannes Altmanninger [off-list ref] wrote:

quoted

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

quoted

quoted

+             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.