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

[Elijah Newren <newren@gmail.com>]

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:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Since some callers may want to send warning messages to somewhere other
> > than stdout/stderr, stop printing "warning: Cannot merge binary files"
> > from ll-merge and instead modify the return status of ll_merge() to
> > indicate when a merge of binary files has occurred.
> >
> > This commit continues printing the message as-is; future changes will
> > start handling the new commit differently in the merge-ort codepath.
>
> "the new commit" looks like a typo, do you mean "binary conflicts"?

Good catch, yeah should be "the binary conflicts message"

> >
> > Note that my methodology included first modifying ll_merge() to return
> > a struct, so that the compiler would catch all the callers for me and
> > ensure I had modified all of them.  After modifying all of them, I then
> > changed the struct to an enum.
>
> Heh, this is a clever way to work around C's weak typing.
>
> The language server I'm using (clangd) supports the Call Hierarchy feature,
> which is intended to list callers or callees of the function at the editor's
> cursor. If I ask the server for callers of ll_merge I get this response
> (on 510f9eba9 plus this series)
>
>         ll-merge.h:98:1: ll_merge - list of callers
>           builtin/checkout.c:242:12: checkout_merged
>             builtin/checkout.c:279:17:  merge_status = ll_merge(&result_buf, path, &ancestor, "base",
>           rerere.c:943:12: handle_cache
>             rerere.c:984:2:     ll_merge(&result, path, &mmfile[0], NULL,
>           notes-merge.c:342:12: ll_merge_in_worktree
>             notes-merge.c:353:11:       status = ll_merge(&result_buf, oid_to_hex(&p->obj), &base, NULL,
>           merge-recursive.c:1035:12: merge_3way
>             merge-recursive.c:1090:17:  merge_status = ll_merge(result_buf, a->path, &orig, base,
>           merge-ort.c:1763:12: merge_3way
>             merge-ort.c:1816:17:        merge_status = ll_merge(result_buf, path, &orig, base,
>           merge-blobs.c:32:14: three_way_filemerge
>             merge-blobs.c:48:17:        merge_status = ll_merge(&res, path, base, NULL,
>           apply.c:3491:12: three_way_merge
>             apply.c:3511:11:    status = ll_merge(&result, path,
>           rerere.c:608:12: try_merge
>             rerere.c:623:9:             ret = ll_merge(result, path, &base, NULL, cur, "", &other, "",
>
> 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.

> > @@ -133,10 +133,12 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
> >       xmp.ancestor = orig_name;
> >       xmp.file1 = name1;
> >       xmp.file2 = name2;
> > -     return xdl_merge(orig, src1, src2, &xmp, result);
> > +     status = xdl_merge(orig, src1, src2, &xmp, result);
> > +     ret = (status > 1 ) ? LL_MERGE_CONFLICT : status;
>
> " (status > 1 )" has an extra space
>
> I'm not sure it's wise to handle status=1 and status=2 in two different code paths.
> Both mean the same (the only difference is the number of conflicts).
> status=1 coincides with LL_MERGE_CONFLICT but that's purely coincidental
>
>         ret = (status > 0) ? LL_MERGE_CONFLICT : status;

Um, whoops.  Yeah, this should be > 0, not > 1.  (As per
xdl_do_merge() comment, status >= 0 means status returns the number of
conflicts)  No clue how I messed that up so badly; kind of
embarrassing, honestly.

Thanks for the careful reading.

> > @@ -236,7 +239,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
> >               unlink_or_warn(temp[i]);
> >       strbuf_release(&cmd);
> >       strbuf_release(&path_sq);
> > -     return status;
> > +     ret = (status > 1) ? LL_MERGE_CONFLICT : status;
>
> same here, I'd test for "status > 0" because that's the convention for
> external programs

Yep.

...
> > diff --git a/rerere.c b/rerere.c
> > index d83d58df4fb..b1f8961ed9e 100644
> > --- a/rerere.c
> > +++ b/rerere.c
> > @@ -609,19 +609,23 @@ static int try_merge(struct index_state *istate,
> >                    const struct rerere_id *id, const char *path,
> >                    mmfile_t *cur, mmbuffer_t *result)
> >  {
> > -     int ret;
> > +     enum ll_merge_result ret;
> >       mmfile_t base = {NULL, 0}, other = {NULL, 0};
> >
> >       if (read_mmfile(&base, rerere_path(id, "preimage")) ||
> > -         read_mmfile(&other, rerere_path(id, "postimage")))
> > -             ret = 1;
> > -     else
> > +         read_mmfile(&other, rerere_path(id, "postimage"))) {
> > +             ret = LL_MERGE_CONFLICT;
> > +     } else {
> >               /*
> >                * A three-way merge. Note that this honors user-customizable
> >                * low-level merge driver settings.
> >                */
> >               ret = ll_merge(result, path, &base, NULL, cur, "", &other, "",
> >                              istate, NULL);
> > +             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).

[1] https://lore.kernel.org/git/YVOZRhWttzF18Xql@coredump.intra.peff.net/