git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Mar 1, 2019, 15:00 UTC
Message-ID
<22878dbe-d9f0-4737-3a49-ece695c2d0c2@gmail.com>
In-Reply-To
<nycvar.QRO.7.76.6.1903011415220.41@tvgsbejvaqbjf.bet>
Hi Dscho
On 01/03/2019 13:19, Johannes Schindelin wrote:
Show 28 quoted lines
> 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.

Show 11 quoted lines
> 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
Show 32 quoted lines
> 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;
>>>
>>
Previous: Johannes SchindelinNext: Junio C Hamano
Message 8 of 16 in “Fix ORIG_HEAD behavior of the built-in rebase”
  1. 0/4 Fix ORIG_HEAD behavior of the built-in rebaseJohannes Schindelin via GitGitGadget, Feb 28, 2019
  2. 3/4 built-in rebase: demonstrate that ORIG_HEAD is not set correctlyJohannes Schindelin via GitGitGadget, Feb 28, 2019
  3. 4/4 built-in rebase: set ORIG_HEAD just once, before the rebaseJohannes Schindelin via GitGitGadget, Feb 28, 2019
  4. 2/4 built-in rebase: use the correct reflog when switching branchesJohannes Schindelin via GitGitGadget, Feb 28, 2019
  5. 1/4 built-in rebase: no need to check out `onto` twiceJohannes Schindelin via GitGitGadget, Feb 28, 2019
  6. Phillip WoodFeb 28, 2019
  7. Johannes SchindelinMar 1, 2019
  8. Phillip WoodMar 1, 2019
  9. Junio C HamanoMar 3, 2019
  10. Johannes SchindelinMar 3, 2019
  11. Phillip WoodMar 3, 2019
  12. 0/4 Fix ORIG_HEAD behavior of the built-in rebaseJohannes Schindelin via GitGitGadget, Mar 3, 2019
  13. 1/4 built-in rebase: no need to check out `onto` twiceJohannes Schindelin via GitGitGadget, Mar 3, 2019
  14. 2/4 built-in rebase: use the correct reflog when switching branchesJohannes Schindelin via GitGitGadget, Mar 3, 2019
  15. 3/4 built-in rebase: demonstrate that ORIG_HEAD is not set correctlyJohannes Schindelin via GitGitGadget, Mar 3, 2019
  16. 4/4 built-in rebase: set ORIG_HEAD just once, before the rebaseJohannes Schindelin via GitGitGadget, Mar 3, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.