Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice
- From
- Phillip 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; >>> >>