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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 19, 2022, 16:41 UTC
Message-ID: <220119.86h79zmzy5.gmgdl@evledraar.gmail.com>
In-Reply-To: <b952f674df13a1c3e4bffa823490f905e862ef7c.1640907369.git.gitgitgadget@gmail.com>

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

Show 25 quoted lines

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