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

On Wed, Jan 19, 2022 at 8:51 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Thu, Dec 30 2021, Elijah Newren via GitGitGadget wrote:

quoted

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.
[...]
-int ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
           const char *path,
           mmfile_t *ancestor, const char *ancestor_label,
           mmfile_t *ours, const char *our_label,

diff --git a/ll-merge.h b/ll-merge.h
index aceb1b24132..e4a20e81a3a 100644
--- a/ll-merge.h
+++ b/ll-merge.h

@@ -82,13 +82,20 @@ struct ll_merge_options {
      long xdl_opts;
 };

+enum ll_merge_result {
+     LL_MERGE_ERROR = -1,
+     LL_MERGE_OK = 0,
+     LL_MERGE_CONFLICT,
+     LL_MERGE_BINARY_CONFLICT,
+};
+

Isn't the other side of the enum checking missing in many cases?

E.g. ll_ext_merge() returns "enum ll_merge_result" now, and does:

        status = run_command_v_opt(args, RUN_USING_SHELL);
        ret = (status > 0) ? LL_MERGE_CONFLICT : status;

And grepping at the tip of this series shows:

    $ git grep LL_MERGE_OK
    ll-merge.c:             ret = LL_MERGE_OK;
    ll-merge.c:                     ret = LL_MERGE_OK;
    ll-merge.c:                     ret = LL_MERGE_OK;
    ll-merge.h:     LL_MERGE_OK = 0,

Similar for LL_MERGE_CONFLICT, the only one that's used outside of the
file itself and its header is LL_MERGE_BINARY_CONFLICT.

I.e. shouldn't these codepaths:

    git grep -w ll_merge

Be doing a switch() on that new enum? E.g. we lose the type in
three_way_merge() in apply.c, it seems to me that that function should
switch over this new enum, and return the "int" that the callers of
three_way_merge() care about (i.e. just <0, 0, 1, not this enum's -1, 0,
1, 2.

Actually, three_way_merge()'s callers operate on <0, 0, >0; 1 is not
special.  That's also the interface that ll_merge() traditionally
always used, and which still mostly applies, it's just they now want
to differentiate between two of the >0 cases (namely LL_MERGE_CONFLICT
vs. LL_MERGE_BINARY_CONFLICT).

That may sound like a big change, but since every single caller can
just specially check for LL_MERGE_BINARY_CONFLICT as a first step if
they care (some callers don't), and then drop back to using old code
as-is that assumes <0 vs. 0 vs. >0, that seems like a lot simpler
change.  And that's what this patch does.

Trying to convert all these callers over to switch statements seems
like unnecessary churn to me.  Since Junio has already reviewed a
former round of this patch and found it to his liking (modulo a
completely different one-line issue that I since corrected), I think
I'd like to stick with the patch as-is.  If folks feel really strongly
about changing something here, though, I can change the return type of
the ll_*merge() functions back to int.