On Wed, Jan 19, 2022 at 8:51 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
> 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.
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.
