diff mbox series

[1/2] rebase: consolidate clean-up code before leaving reset_head()

Message ID 64597fe827021383ab68cfb247de61fcf104a961.1541756054.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fix built-in rebase perf regression | expand

Commit Message

Bruce Perry via GitGitGadget Nov. 9, 2018, 9:34 a.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

The same clean-up code is repeated quite a few times; Let's DRY up the
code some.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rebase.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Jeff King Nov. 9, 2018, 10:03 a.m. UTC | #1 
On Fri, Nov 09, 2018 at 01:34:17AM -0800, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0ee06aa363..6f6d7de156 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -569,16 +569,13 @@ static int reset_head(struct object_id *oid, const char *action,
>  	}
>  
>  	if (!fill_tree_descriptor(&desc, oid)) {
> -		error(_("failed to find tree of %s"), oid_to_hex(oid));
> -		rollback_lock_file(&lock);
> -		free((void *)desc.buffer);
> -		return -1;
> +		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
> +		goto leave_reset_head;
>  	}

If fill_tree_descriptor() fails, what is left in desc.buffer? Looking at
the implementation, I think it's always NULL or a valid buffer. But I
think all code paths actually die() unless we pass a NULL oid (and in
that case desc.buffer would be NULL, too).

So I think the original here that calls free() doesn't ever do anything
but it did not hurt. After your patch, the leave_reset_head code would
continue to call free(), and that's OK.

There are a few earlier conditionals in reset_head() that do only
rollback_lock_file() that could similarly be converted to use the goto.
But they would need desc.buffer to be initialized to NULL. I could go
either way on converting them or not.

> @@ -586,10 +583,9 @@ static int reset_head(struct object_id *oid, const char *action,
>  
>  	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)
>  		ret = error(_("could not write index"));
> -	free((void *)desc.buffer);
>  
>  	if (ret)
> -		return ret;
> +		goto leave_reset_head;
>  
>  	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
>  	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
> @@ -622,7 +618,10 @@ static int reset_head(struct object_id *oid, const char *action,
>  					 UPDATE_REFS_MSG_ON_ERR);
>  	}
>  
> +leave_reset_head:
>  	strbuf_release(&msg);
> +	rollback_lock_file(&lock);
> +	free((void *)desc.buffer);
>  	return ret;

We get here on success, too. So we may call rollback_lock_file() on an
already-committed lock. This is explicitly documented as a no-op by the
lock code, so that's OK.

So overall looks good to me.

-Peff
Johannes Schindelin Nov. 9, 2018, 5:13 p.m. UTC | #2 
Hi Peff,

On Fri, 9 Nov 2018, Jeff King wrote:

> On Fri, Nov 09, 2018 at 01:34:17AM -0800, Johannes Schindelin via GitGitGadget wrote:
> 
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 0ee06aa363..6f6d7de156 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -569,16 +569,13 @@ static int reset_head(struct object_id *oid, const char *action,
> >  	}
> >  
> >  	if (!fill_tree_descriptor(&desc, oid)) {
> > -		error(_("failed to find tree of %s"), oid_to_hex(oid));
> > -		rollback_lock_file(&lock);
> > -		free((void *)desc.buffer);
> > -		return -1;
> > +		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
> > +		goto leave_reset_head;
> >  	}
> 
> If fill_tree_descriptor() fails, what is left in desc.buffer? Looking at
> the implementation, I think it's always NULL or a valid buffer. But I
> think all code paths actually die() unless we pass a NULL oid (and in
> that case desc.buffer would be NULL, too).
> 
> So I think the original here that calls free() doesn't ever do anything
> but it did not hurt. After your patch, the leave_reset_head code would
> continue to call free(), and that's OK.

Right, that was my thinking, too.

> There are a few earlier conditionals in reset_head() that do only
> rollback_lock_file() that could similarly be converted to use the goto.
> But they would need desc.buffer to be initialized to NULL. I could go
> either way on converting them or not.

Whoops. I should have checked more carefully?

> > @@ -586,10 +583,9 @@ static int reset_head(struct object_id *oid, const char *action,
> >  
> >  	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)
> >  		ret = error(_("could not write index"));
> > -	free((void *)desc.buffer);
> >  
> >  	if (ret)
> > -		return ret;
> > +		goto leave_reset_head;
> >  
> >  	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> >  	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
> > @@ -622,7 +618,10 @@ static int reset_head(struct object_id *oid, const char *action,
> >  					 UPDATE_REFS_MSG_ON_ERR);
> >  	}
> >  
> > +leave_reset_head:
> >  	strbuf_release(&msg);
> > +	rollback_lock_file(&lock);
> > +	free((void *)desc.buffer);
> >  	return ret;
> 
> We get here on success, too. So we may call rollback_lock_file() on an
> already-committed lock. This is explicitly documented as a no-op by the
> lock code, so that's OK.

Indeed. I did not check the documentation, but the code, and came to the
same conclusion.

> So overall looks good to me.

Thanks!
Dscho
diff mbox series

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..6f6d7de156 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -569,16 +569,13 @@  static int reset_head(struct object_id *oid, const char *action,
 	}
 
 	if (!fill_tree_descriptor(&desc, oid)) {
-		error(_("failed to find tree of %s"), oid_to_hex(oid));
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		return -1;
+		ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+		goto leave_reset_head;
 	}
 
 	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
-		rollback_lock_file(&lock);
-		free((void *)desc.buffer);
-		return -1;
+		ret = -1;
+		goto leave_reset_head;
 	}
 
 	tree = parse_tree_indirect(oid);
@@ -586,10 +583,9 @@  static int reset_head(struct object_id *oid, const char *action,
 
 	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)
 		ret = error(_("could not write index"));
-	free((void *)desc.buffer);
 
 	if (ret)
-		return ret;
+		goto leave_reset_head;
 
 	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
 	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
@@ -622,7 +618,10 @@  static int reset_head(struct object_id *oid, const char *action,
 					 UPDATE_REFS_MSG_ON_ERR);
 	}
 
+leave_reset_head:
 	strbuf_release(&msg);
+	rollback_lock_file(&lock);
+	free((void *)desc.buffer);
 	return ret;
 }