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:
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxxxx/
