Re: [PATCH v3 3/9] ll-merge: make callers responsible for showing warnings
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-01-19 16:51:20
On Thu, Dec 30 2021, Elijah Newren via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
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.