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

From: Jeff King <peff@peff.net>
Date: Nov 9, 2018, 10:03 UTC
Message-ID: <20181109100357.GA7410@sigill.intra.peff.net>
In-Reply-To: <64597fe827021383ab68cfb247de61fcf104a961.1541756054.git.gitgitgadget@gmail.com>

On Fri, Nov 09, 2018 at 01:34:17AM -0800, Johannes Schindelin via GitGitGadget wrote:

Show 15 quoted lines

> 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.

Show 21 quoted lines

> @@ -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