| Message ID | 9bd34b4a404136c974b71975c7a3d809a874cc3d.1566724236.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: learn --keep-base and improvements on fast-forward behaviour | expand |
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@gmail.com> > --- > 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. > 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'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.
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@gmail.com> > > --- > > 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
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); return res && is_linear_history(onto, head); }
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@gmail.com> --- builtin/rebase.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)