On Sun, Aug 25, 2019 at 06:47:02PM +0530, Pratyush Yadav wrote:
> On 25/08/19 05:12AM, Denton Liu wrote:
> > Before, can_fast_forward was written with an if-else statement. However,
> > in the future, we may be adding more termination cases which would lead
> > to deeply nested if statements.
> >
> > Refactor to use a goto tower so that future cases can be easily
> > inserted.
> >
> > Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> > ---
> > builtin/rebase.c | 24 +++++++++++++++---------
> > 1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 670096c065..22c4f1ff93 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1264,21 +1264,27 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
> > struct object_id *merge_base)
> > {
> > struct commit *head = lookup_commit(the_repository, head_oid);
> > - struct commit_list *merge_bases;
> > - int res;
> > + struct commit_list *merge_bases = NULL;
> > + int res = 0;
> >
> > if (!head)
> > - return 0;
> > + goto done;
> >
> > merge_bases = get_merge_bases(onto, head);
> > - if (merge_bases && !merge_bases->next) {
> > - oidcpy(merge_base, &merge_bases->item->object.oid);
> > - res = oideq(merge_base, &onto->object.oid);
> > - } else {
> > + if (!merge_bases || merge_bases->next) {
> > oidcpy(merge_base, &null_oid);
> > - res = 0;
> > + goto done;
> > }
> > - free_commit_list(merge_bases);
> > +
> > + oidcpy(merge_base, &merge_bases->item->object.oid);
> > + if (!oideq(merge_base, &onto->object.oid))
> > + goto done;
> > +
> > + res = 1;
> > +
> > +done:
> > + if (merge_bases)
> > + free_commit_list(merge_bases);
>
> free_commit_list() returns immediately when a NULL pointer is passed in,
> so I'm not sure if this check is really necessary. I think it is a
> reasonable assumption to make that free* functions work well with NULL
> inputs.
I didn't realise that free_commit_list() would freely accept a NULL
pointer without segfaulting. I'll remove the surrounding if.
>
> > return res && is_linear_history(onto, head);
> > }
> >
>
> Out of curiosity, since you are going with a goto tower, why not do
> something like:
>
> done_merge_bases:
> free_commit_list(merge_bases);
> done:
> return res && is_linear_history(onto, head);
>
> You jump to done_merge_bases after you have initialized merge_bases, and
> directly to done before initializing it.
I opted to do it this way since I figured it was less complexity to have
a common jump target at the end. Also, in case in the future we decided
to add more logic before merge_bases was initialised, we wouldn't have
to worry about figuring out which target to jump to.
>
> I'm not advocating for either way, just curious if there is a specific
> reason to do it your way. Anyway, if you drop the if (merge_bases), then
> it doesn't really matter I suppose.
I'm pretty impartial as well. I'll drop the if and leave the rest as is
unless someone else feels strongly about this.
>
> --
> Regards,
> Pratyush Yadav
