[v9,5/9] rebase: refactor can_fast_forward into goto tower
diff mbox series

Message ID 9bd34b4a404136c974b71975c7a3d809a874cc3d.1566724236.git.liu.denton@gmail.com
State New
Headers show
Series
  • rebase: learn --keep-base and improvements on fast-forward behaviour
Related show

Commit Message

Denton Liu Aug. 25, 2019, 9:12 a.m. UTC
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(-)

Comments

Pratyush Yadav Aug. 25, 2019, 1:17 p.m. UTC | #1
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.
Denton Liu Aug. 26, 2019, 11:17 p.m. UTC | #2
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

Patch
diff mbox series

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);
 }