On Thu, Dec 30 2021, Elijah Newren via GitGitGadget wrote:
> 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.
