git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Jeff King <peff@peff.net>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/2] rebase: consolidate clean-up code before leaving reset_head()
Date: Fri, 9 Nov 2018 18:13:08 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1811091750330.39@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20181109100357.GA7410@sigill.intra.peff.net>

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

  reply	other threads:[~2018-11-09 17:13 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-09  9:34 [PATCH 0/2] Fix built-in rebase perf regression Johannes Schindelin via GitGitGadget
2018-11-09  9:34 ` [PATCH 1/2] rebase: consolidate clean-up code before leaving reset_head() Johannes Schindelin via GitGitGadget
2018-11-09 10:03   ` Jeff King
2018-11-09 17:13     ` Johannes Schindelin [this message]
2018-11-09  9:34 ` [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate Johannes Schindelin via GitGitGadget
2018-11-09 10:11   ` Jeff King
2018-11-09 17:21     ` Johannes Schindelin
2018-11-09 19:59       ` Jeff King
2018-11-12 11:11         ` Johannes Schindelin
2018-11-12  4:04       ` Junio C Hamano
2018-11-12 11:42         ` Johannes Schindelin
2018-11-13  1:49           ` Junio C Hamano
2018-11-13  6:07             ` Junio C Hamano
2018-11-12 11:44 ` [PATCH v2 0/3] Fix built-in rebase perf regression Johannes Schindelin via GitGitGadget
2018-11-12 11:44   ` [PATCH v2 1/3] rebase: consolidate clean-up code before leaving reset_head() Johannes Schindelin via GitGitGadget
2018-11-12 11:44   ` [PATCH v2 2/3] rebase: prepare reset_head() for more flags Johannes Schindelin via GitGitGadget
2018-11-12 11:44   ` [PATCH v2 3/3] built-in rebase: reinstate `checkout -q` behavior where appropriate Johannes Schindelin via GitGitGadget
2018-11-12 14:26   ` [PATCH v2 0/3] Fix built-in rebase perf regression Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1811091750330.39@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).