Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Dscho

On 01/03/2019 13:19, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Thu, 28 Feb 2019, Phillip Wood wrote:
> 
>> On 28/02/2019 15:27, Johannes Schindelin via GitGitGadget wrote:
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>
>>> In the case that the rebase boils down to a fast-forward, the built-in
>>> rebase reset the working tree twice: once to start the rebase at `onto`,
>>> then realizing that the original HEAD was an ancestor, `reset_head()`
>>> was called to update the original ref and to point HEAD back to it.
>>>
>>> That second `reset_head()` call does not need to touch the working tree,
>>> though, as it does not change the actual tip commit. So let's avoid that
>>> unnecessary work.
>>
>> I'm confused by this I think I must be missing something. If we've checked out
>> onto then why does the working copy not need updating when we fast forward.
>> (also why to we checkout onto before seeing if we can fast-forward but that's
>> not related to this patch series)
> 
> Sorry, I really try to learn how to express things clearly. Still learning
> a lot.
> 
> So what happens is this: we detect the situation where the pre-rebase
> `HEAD` is an ancestor of `onto`. We do this *after* checking out `onto`.
> So we now know that we essentially fast-forwarded to the post-rebase
> state.

Ah that makes sense, for some reason I misread the commit message and 
thought that we were dealing with the case where onto was an ancestor of 
the original HEAD and we were fast-forwarding from onto back to the 
original HEAD.

> What we still need to do is to update the original ref (unless we were on
> a detached HEAD when the rebase was started).
> 
> The original shell code updates the original ref to the current HEAD, and
> the updates HEAD to point to that symbolic ref instead of being detached.
> 
> In the C code, we abused `reset_head()` to do the same. But `reset_head()`
> does more: it does a two-way merge (in this instance, because
> `RESET_HEAD_HARD` was not part of the flags). Which is unnecessary.
> 
> That's all this commit is about.

Thanks for explaining, it all makes sense to me now

Best Wishes

Phillip

> Ciao,
> Dscho
> 
>>
>> Best Wishes
>>
>> Phillip
>>
>>>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> ---
>>>    builtin/rebase.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>>> index 08ec4d52c7..813ec284ca 100644
>>> --- a/builtin/rebase.c
>>> +++ b/builtin/rebase.c
>>> @@ -1740,8 +1740,8 @@ int cmd_rebase(int argc, const char **argv, const char
>>> *prefix)
>>>      strbuf_addf(&msg, "rebase finished: %s onto %s",
>>>       options.head_name ? options.head_name : "detached HEAD",
>>>       oid_to_hex(&options.onto->object.oid));
>>> -		reset_head(NULL, "Fast-forwarded", options.head_name, 0,
>>> -			   "HEAD", msg.buf);
>>> +		reset_head(NULL, "Fast-forwarded", options.head_name,
>>> +			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf);
>>>      strbuf_release(&msg);
>>>      ret = !!finish_rebase(&options);
>>>      goto cleanup;
>>>
>>