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

Commit Message

Francesco Guastella via GitGitGadget Feb. 28, 2019, 3:27 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Phillip Wood Feb. 28, 2019, 8:55 p.m. UTC | #1

Hi Johannes

On 28/02/2019 15:27, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> 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)

Best Wishes

Phillip

> 
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>   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;
>

Johannes Schindelin March 1, 2019, 1:19 p.m. UTC | #2

Hi Phillip,

On Thu, 28 Feb 2019, Phillip Wood wrote:

> On 28/02/2019 15:27, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[e-mail]>
> > 
> > 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.

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.

Ciao,
Dscho

> 
> Best Wishes
> 
> Phillip
> 
> > 
> > Signed-off-by: Johannes Schindelin <[e-mail]>
> > ---
> >   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;
> > 
>

Phillip Wood March 1, 2019, 3 p.m. UTC | #3

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> ---
>>>    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;
>>>
>>

Junio C Hamano March 3, 2019, 1:35 a.m. UTC | #4

Phillip Wood <[e-mail]> writes:

> Thanks for explaining, it all makes sense to me now

It would be necessary to make sure that it all makes sense to all
future readers.  Are they patches good enough as-is for that, or do
they need some updates before I take a look at them to pick up?

Johannes Schindelin March 3, 2019, 5:09 p.m. UTC | #5

Hi Junio,

On Sun, 3 Mar 2019, Junio C Hamano wrote:

> Phillip Wood <[e-mail]> writes:
> 
> > Thanks for explaining, it all makes sense to me now
> 
> It would be necessary to make sure that it all makes sense to all
> future readers.  Are they patches good enough as-is for that, or do
> they need some updates before I take a look at them to pick up?

As you frequently say: if a reviewer gets puzzled, it is often a good
indicator that the commit message needs to be improved. I did exactly that
and will send out the next iteration in a minute.

Ciao,
Dscho

Phillip Wood March 3, 2019, 7:11 p.m. UTC | #6

Hi Junio

On 03/03/2019 01:35, Junio C Hamano wrote:
> Phillip Wood <[e-mail]> writes:
> 
>> Thanks for explaining, it all makes sense to me now
> 
> It would be necessary to make sure that it all makes sense to all
> future readers.  Are they patches good enough as-is for that, or do
> they need some updates before I take a look at them to pick up?
> 

I've just re-read them and I think they're fine as is, though the first
 paragraph of the first commit message might be clearer if "HEAD was an
ancestor and" was changed to "HEAD was an ancestor of onto and".

Best Wishes

Phillip

Patch

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;