Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate

From: Jeff King <peff@peff.net>
Date: Nov 9, 2018, 10:11 UTC
Message-ID: <20181109101148.GB7410@sigill.intra.peff.net>
In-Reply-To: <070092b4309e5e74e3a1b3be54613cccf26e97da.1541756054.git.gitgitgadget@gmail.com>

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

Show 11 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> When we converted a `git checkout -q $onto^0` call to use
> `reset_head()`, we inadvertently incurred a change from a twoway_merge
> to a oneway_merge, as if we wanted a `git reset --hard` instead.
> 
> This has performance ramifications under certain, though, as the
> oneway_merge needs to lstat() every single index entry whereas
> twoway_merge does not.
> 
> So let's go back to the old behavior.

Makes sense. I didn't think too hard about any possible gotchas with the twoway/oneway switch, but if that's what git-checkout was doing before, it seems obviously safe.

Show 11 quoted lines

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6f6d7de156..c1cc50f3f8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -523,11 +523,12 @@ finished_rebase:
>  #define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
>  
>  static int reset_head(struct object_id *oid, const char *action,
> -		      const char *switch_to_branch, int detach_head,
> +		      const char *switch_to_branch,
> +		      int detach_head, int reset_hard,

It might be worth switching to a single flag variable here. It would make calls like this:

> -	if (reset_head(&options.onto->object.oid, "checkout", NULL, 1,
> +	if (reset_head(&options.onto->object.oid, "checkout", NULL, 1, 0,
>  	    NULL, msg.buf))

a little more self-documenting (if a little more verbose).

Show 10 quoted lines

> -	if (!oid) {
> -		if (get_oid("HEAD", &head_oid)) {
> -			rollback_lock_file(&lock);
> -			return error(_("could not determine HEAD revision"));
> -		}
> -		oid = &head_oid;
> +	if (get_oid("HEAD", &head_oid)) {
> +		rollback_lock_file(&lock);
> +		return error(_("could not determine HEAD revision"));
>  	}

This one could actually turn into:

  ret = error(...);
  goto leave_reset_head;

now. We don't have to worry about an uninitialized desc.buffer anymore (as I mentioned in the previous email), because "nr" would be 0.

It doesn't save any lines, though (but maybe having a single cleanup/exit point would make things easier to read; I dunno).

Take all my comments as observations, not objections. This looks OK to me either way.

-Peff