Re: [PATCH 2/9] ll-merge: make callers responsible for showing warnings

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
> >
> > 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.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  apply.c            |  5 ++++-
> >  builtin/checkout.c | 12 ++++++++----
> >  ll-merge.c         | 40 ++++++++++++++++++++++------------------
> >  ll-merge.h         |  9 ++++++++-
> >  merge-blobs.c      |  5 ++++-
> >  merge-ort.c        |  5 ++++-
> >  merge-recursive.c  |  5 ++++-
> >  notes-merge.c      |  5 ++++-
> >  rerere.c           | 10 +++++++---
> >  9 files changed, 65 insertions(+), 31 deletions(-)
> >
> > diff --git a/apply.c b/apply.c
> > index 43a0aebf4ee..12ea9c72a6b 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -3492,7 +3492,7 @@ static int three_way_merge(struct apply_state *state,
> >  {
> >       mmfile_t base_file, our_file, their_file;
> >       mmbuffer_t result = { NULL };
> > -     int status;
> > +     enum ll_merge_result status;
> >
> >       /* resolve trivial cases first */
> >       if (oideq(base, ours))
> > @@ -3509,6 +3509,9 @@ static int three_way_merge(struct apply_state *state,
> >                         &their_file, "theirs",
> >                         state->repo->index,
> >                         NULL);
> > +     if (status == LL_MERGE_BINARY_CONFLICT)
> > +             warning("Cannot merge binary files: %s (%s vs. %s)",
> > +                     "base", "ours", "theirs");
>
> This used to come from ll_merge()
>
> > -                     warning("Cannot merge binary files: %s (%s vs. %s)",
> > -                             path, name1, name2);
> > -                     /* fallthru */
>
> And our call to ll_merge() above (half of it invisible in the
> pre-context of the hunk) gave "ours" and "theirs" to our_label and
> their_label, which in turn are called name1 and name2, respectively,
> in ll_merge_binary() driver.
>
> I am not sure about the "base" string, though.  I suspect that your
> "base" should be a reference to the parameter 'path' of three_way_merge()
> function.

> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index cbf73b8c9f6..3a559d69303 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -237,6 +237,7 @@ static int checkout_merged(int pos, const struct checkout *state,
> >       struct cache_entry *ce = active_cache[pos];
> >       const char *path = ce->name;
> >       mmfile_t ancestor, ours, theirs;
> > +     enum ll_merge_result merge_status;
> >       int status;
> >       struct object_id oid;
> >       mmbuffer_t result_buf;
> > @@ -267,13 +268,16 @@ static int checkout_merged(int pos, const struct checkout *state,
> >       memset(&ll_opts, 0, sizeof(ll_opts));
> >       git_config_get_bool("merge.renormalize", &renormalize);
> >       ll_opts.renormalize = renormalize;
> > -     status = ll_merge(&result_buf, path, &ancestor, "base",
> > -                       &ours, "ours", &theirs, "theirs",
> > -                       state->istate, &ll_opts);
> > +     merge_status = ll_merge(&result_buf, path, &ancestor, "base",
> > +                             &ours, "ours", &theirs, "theirs",
> > +                             state->istate, &ll_opts);
> >       free(ancestor.ptr);
> >       free(ours.ptr);
> >       free(theirs.ptr);
> > -     if (status < 0 || !result_buf.ptr) {
> > +     if (merge_status == LL_MERGE_BINARY_CONFLICT)
> > +             warning("Cannot merge binary files: %s (%s vs. %s)",
> > +                     path, "ours", "theirs");
>
> This one looks correct.
>
> > +     if (merge_status < 0 || !result_buf.ptr) {
> >               free(result_buf.ptr);
> >               return error(_("path '%s': cannot merge"), path);
> >       }
>
> > diff --git a/merge-blobs.c b/merge-blobs.c
> > index ee0a0e90c94..8138090f81c 100644
> > --- a/merge-blobs.c
> > +++ b/merge-blobs.c
> > @@ -36,7 +36,7 @@ static void *three_way_filemerge(struct index_state *istate,
> >                                mmfile_t *their,
> >                                unsigned long *size)
> >  {
> > -     int merge_status;
> > +     enum ll_merge_result merge_status;
> >       mmbuffer_t res;
> >
> >       /*
> > @@ -50,6 +50,9 @@ static void *three_way_filemerge(struct index_state *istate,
> >                               istate, NULL);
> >       if (merge_status < 0)
> >               return NULL;
> > +     if (merge_status == LL_MERGE_BINARY_CONFLICT)
> > +             warning("Cannot merge binary files: %s (%s vs. %s)",
> > +                     path, ".our", ".their");
>
> OK.
>
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 0342f104836..c24da2ba3cb 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -1743,7 +1743,7 @@ static int merge_3way(struct merge_options *opt,
> >       mmfile_t orig, src1, src2;
> >       struct ll_merge_options ll_opts = {0};
> >       char *base, *name1, *name2;
> > -     int merge_status;
> > +     enum ll_merge_result merge_status;
> >
> >       if (!opt->priv->attr_index.initialized)
> >               initialize_attr_index(opt);
> > @@ -1787,6 +1787,9 @@ static int merge_3way(struct merge_options *opt,
> >       merge_status = ll_merge(result_buf, path, &orig, base,
> >                               &src1, name1, &src2, name2,
> >                               &opt->priv->attr_index, &ll_opts);
> > +     if (merge_status == LL_MERGE_BINARY_CONFLICT)
> > +             warning("Cannot merge binary files: %s (%s vs. %s)",
> > +                     path, name1, name2);
>
> OK; this is your code and I do not have to read it too carefully,
> but all we need is conveniently in the pre-context of the hunk ;-).
>
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index d9457797dbb..bc73c52dd84 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -1044,7 +1044,7 @@ static int merge_3way(struct merge_options *opt,
> >       mmfile_t orig, src1, src2;
> >       struct ll_merge_options ll_opts = {0};
> >       char *base, *name1, *name2;
> > -     int merge_status;
> > +     enum ll_merge_result merge_status;
> >
> >       ll_opts.renormalize = opt->renormalize;
> >       ll_opts.extra_marker_size = extra_marker_size;
> > @@ -1090,6 +1090,9 @@ static int merge_3way(struct merge_options *opt,
> >       merge_status = ll_merge(result_buf, a->path, &orig, base,
> >                               &src1, name1, &src2, name2,
> >                               opt->repo->index, &ll_opts);
> > +     if (merge_status == LL_MERGE_BINARY_CONFLICT)
> > +             warning("Cannot merge binary files: %s (%s vs. %s)",
> > +                     a->path, name1, name2);
>
> OK.
>
> > diff --git a/notes-merge.c b/notes-merge.c
> > index b4a3a903e86..01d596920ea 100644
> > --- a/notes-merge.c
> > +++ b/notes-merge.c
> > @@ -344,7 +344,7 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,
> >  {
> >       mmbuffer_t result_buf;
> >       mmfile_t base, local, remote;
> > -     int status;
> > +     enum ll_merge_result status;
> >
> >       read_mmblob(&base, &p->base);
> >       read_mmblob(&local, &p->local);
> > @@ -358,6 +358,9 @@ static int ll_merge_in_worktree(struct notes_merge_options *o,
> >       free(local.ptr);
> >       free(remote.ptr);
> >
> > +     if (status == LL_MERGE_BINARY_CONFLICT)
> > +             warning("Cannot merge binary files: %s (%s vs. %s)",
> > +                     oid_to_hex(&p->obj), o->local_ref, o->remote_ref);
>
> This uses another slot in the rotating buffer used by oid_to_hex(),
> but I do not think anybody grabbed a pointer to one of them and held
> onto it before we got here, so it would be OK.
>
> > diff --git a/rerere.c b/rerere.c
> > index d83d58df4fb..46fd01819b8 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
> > +             ret = LL_MERGE_CONFLICT;
> > +     else {
>
> Let's have {} around the if clause now the corresponding else clause
> needs it.

> >               /*
> >                * 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, "", "");
> > +     }
>
> This is a faithful conversion of what should not happen in practice,
> as the rerere logic would not be able to reach here.  In a binary
> file, we won't be able to identify <<< === >>> blocks, hash the text
> in the conflicted block to come up with the conflict ID to find the
> preimage and postimage files.  These files are the input to the low
> level merge driver call we are making here.
>
> Looking almost good except for a warning message bug I spotted
> earlier.
>
> Thanks.